diff --git a/.paw/work/feature-purl/CodeResearch.md b/.paw/work/feature-purl/CodeResearch.md new file mode 100644 index 0000000..d9c23d9 --- /dev/null +++ b/.paw/work/feature-purl/CodeResearch.md @@ -0,0 +1,1374 @@ +# Code Research: Subpath Skill Selection + +Deep implementation-focused analysis of the craft codebase for the subpath skill selection feature. This document specifies exact code changes, function signatures, type definitions, and a phasing strategy. + +--- + +## 1. DepURL Parser Changes + +### Current ParseDepURL (depurl.go:72–114) + +```go +func ParseDepURL(raw string) (*DepURL, error) { + atIdx := strings.Index(raw, "@") + // ... splits on @, classifies ref +} +``` + +Current DepURL struct (depurl.go:40–63) has 7 fields: `Raw`, `Host`, `Org`, `Repo`, `Version`, `Ref`, `RefType`. + +### Exact Modified DepURL Struct + +```go +type DepURL struct { + Raw string + Host string + Org string + Repo string + Version string + Ref string + RefType RefType + Subpath string // NEW: optional subpath fragment (e.g., "skills/docx") +} +``` + +### Exact Modified ParseDepURL + +The `#` fragment must be split **before** the `@` split because the URL format is `host/org/repo@ref#subpath`. The `#` appears after the ref, so splitting on `#` first from the raw string cleanly separates the subpath from the rest. + +```go +func ParseDepURL(raw string) (*DepURL, error) { + // NEW: Extract optional #subpath fragment before any other parsing. + var subpath string + if hashIdx := strings.Index(raw, "#"); hashIdx >= 0 { + subpath = raw[hashIdx+1:] + raw = raw[:hashIdx] + } + + atIdx := strings.Index(raw, "@") + if atIdx < 0 { + return nil, fmt.Errorf("invalid dependency URL %q: missing '@' ...", raw) + } + + identity := raw[:atIdx] + ref := raw[atIdx+1:] + // ... existing ref classification logic unchanged ... + + d := &DepURL{ + Raw: raw, // NOTE: Raw stores the fragment-stripped URL + Host: matches[1], + Org: matches[2], + Repo: matches[3], + Subpath: normalizeSubpath(subpath), // NEW + } + // ... existing ref type logic unchanged ... + return d, nil +} +``` + +### Subpath Normalization Helper + +```go +// normalizeSubpath cleans a subpath fragment: strips leading "./", trailing "/", +// and returns empty string for empty/whitespace-only input. +func normalizeSubpath(s string) string { + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "./") + s = strings.TrimSuffix(s, "/") + return s +} +``` + +### String() Method Change (depurl.go:173–178) + +Current `String()` returns `d.Raw` if set, else reconstructs. With the change, `Raw` stores the fragment-stripped URL. The reconstructed form must append `#subpath` if present: + +```go +func (d *DepURL) String() string { + base := d.Raw + if base == "" { + base = d.PackageIdentity() + "@" + d.RefString() + } + if d.Subpath != "" { + return base + "#" + d.Subpath + } + return base +} +``` + +### WithVersion() Behavior (depurl.go:167–170) + +`WithVersion()` returns a bare URL for version comparison. Subpath should be **dropped** — this method is used for manifest URL updates (update.go:161) where the subpath is tracked in `DependencySpec.Select`, not in the URL itself. + +Current implementation is already correct — no change needed: +```go +func (d *DepURL) WithVersion(version string) string { + version = strings.TrimPrefix(version, "v") + return d.PackageIdentity() + "@v" + version +} +``` + +### Edge Case: `github.com/org/repo@branch:feat/thing#sub/path` + +The `#` character is unambiguous here. The parsing order is: +1. Split on first `#` → `github.com/org/repo@branch:feat/thing` + `sub/path` +2. Split on first `@` → `github.com/org/repo` + `branch:feat/thing` +3. `branch:feat/thing` is correctly parsed as a branch ref (slashes are valid in branch names) + +The only ambiguity would be a branch name **containing** `#`, which the spec explicitly declares as unsupported (Spec.md edge cases, line 85: "Branch name containing `#` character: unsupported, documented limitation"). + +### Test Changes (depurl_test.go) + +New test cases to add to the table-driven `TestParseDepURL`: +```go +{ + name: "tag with subpath", + input: "github.com/acme/skills@v1.0.0#skills/docx", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "skills/docx", + }, +}, +{ + name: "commit with subpath", + input: "github.com/acme/tools@abc1234#tools/x", + want: &DepURL{ + Raw: "github.com/acme/tools@abc1234", + Host: "github.com", + Org: "acme", + Repo: "tools", + Ref: "abc1234", + RefType: RefTypeCommit, + Subpath: "tools/x", + }, +}, +{ + name: "branch with subpath", + input: "github.com/acme/tools@branch:main#tools/x", + want: &DepURL{ + Raw: "github.com/acme/tools@branch:main", + Host: "github.com", + Org: "acme", + Repo: "tools", + Ref: "main", + RefType: RefTypeBranch, + Subpath: "tools/x", + }, +}, +{ + name: "nested subpath", + input: "github.com/acme/skills@v1.0.0#skills/nested/docx", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "skills/nested/docx", + }, +}, +{ + name: "empty fragment treated as no subpath", + input: "github.com/acme/skills@v1.0.0#", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "", + }, +}, +{ + name: "subpath with leading dot-slash normalized", + input: "github.com/acme/skills@v1.0.0#./skills/docx", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "skills/docx", + }, +}, +{ + name: "branch with slashes and subpath", + input: "github.com/org/repo@branch:feat/thing#sub/path", + want: &DepURL{ + Raw: "github.com/org/repo@branch:feat/thing", + Host: "github.com", + Org: "org", + Repo: "repo", + Ref: "feat/thing", + RefType: RefTypeBranch, + Subpath: "sub/path", + }, +}, +``` + +New `TestDepURLMethods` assertions: +```go +// String() with subpath +d, _ := ParseDepURL("github.com/acme/skills@v1.0.0#skills/docx") +if got := d.String(); got != "github.com/acme/skills@v1.0.0#skills/docx" { + t.Errorf("String() with subpath = %q", got) +} + +// WithVersion drops subpath +if got := d.WithVersion("v2.0.0"); got != "github.com/acme/skills@v2.0.0" { + t.Errorf("WithVersion should not include subpath, got %q", got) +} + +// PackageIdentity excludes subpath +if got := d.PackageIdentity(); got != "github.com/acme/skills" { + t.Errorf("PackageIdentity should exclude subpath, got %q", got) +} +``` + +--- + +## 2. DependencySpec Implementation + +### Exact DependencySpec Struct (types.go) + +Add to `internal/manifest/types.go`: + +```go +// DependencySpec represents a dependency in the manifest. +// It can be serialized as either a simple URL string or a structured +// object with url and select fields. +type DependencySpec struct { + // URL is the dependency URL (always present). + URL string `yaml:"url"` + + // Select lists subpath filters (optional). Empty means all skills. + Select []string `yaml:"select,omitempty"` +} +``` + +### Changed Manifest Struct (types.go:6–27) + +```go +type Manifest struct { + SchemaVersion int `yaml:"schema_version"` + Name string `yaml:"name"` + Description string `yaml:"description,omitempty"` + License string `yaml:"license,omitempty"` + Skills []string `yaml:"skills"` + Dependencies map[string]DependencySpec `yaml:"dependencies,omitempty"` // CHANGED + Metadata map[string]string `yaml:"metadata,omitempty"` +} +``` + +### Exact UnmarshalYAML Method + +```go +func (d *DependencySpec) UnmarshalYAML(value *yaml.Node) error { + switch value.Kind { + case yaml.ScalarNode: + // Simple string form: "github.com/org/repo@v1.0.0" + d.URL = value.Value + return nil + case yaml.MappingNode: + // Structured form: {url: "...", select: [...]} + type raw struct { + URL string `yaml:"url"` + Select []string `yaml:"select"` + } + var r raw + if err := value.Decode(&r); err != nil { + return err + } + if r.URL == "" { + return fmt.Errorf("structured dependency must have a 'url' field") + } + d.URL = r.URL + d.Select = r.Select + return nil + default: + return fmt.Errorf("dependency must be a string or mapping, got %v", value.Tag) + } +} +``` + +This requires adding `"fmt"` and `"gopkg.in/yaml.v3"` imports to `types.go`. + +### Exact MarshalYAML Method + +Not needed as a method on DependencySpec. Instead, the `Write()` function in `write.go` needs a new `addDependencies` helper (replacing the `addStringMap` call on line 32). + +### write.go Changes + +**Line 31–33:** Replace `addStringMap` call with new `addDependencies` call: +```go +// BEFORE: +if len(m.Dependencies) > 0 { + addStringMap(mapping, "dependencies", m.Dependencies) +} + +// AFTER: +if len(m.Dependencies) > 0 { + addDependencies(mapping, "dependencies", m.Dependencies) +} +``` + +**New function `addDependencies`:** + +```go +// addDependencies serializes the dependency map with format fidelity: +// simple deps (no Select) are written as scalar strings; structured deps +// are written as mapping nodes with url + select fields. +func addDependencies(mapping *yaml.Node, key string, deps map[string]DependencySpec) { + keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key} + mapNode := &yaml.Node{Kind: yaml.MappingNode} + + keys := make([]string, 0, len(deps)) + for k := range deps { + keys = append(keys, k) + } + sort.Strings(keys) + + for _, alias := range keys { + dep := deps[alias] + aliasNode := &yaml.Node{Kind: yaml.ScalarNode, Value: alias} + + if len(dep.Select) == 0 { + // Simple string form + valNode := &yaml.Node{Kind: yaml.ScalarNode, Value: dep.URL} + mapNode.Content = append(mapNode.Content, aliasNode, valNode) + } else { + // Structured form: {url: "...", select: [...]} + objNode := &yaml.Node{Kind: yaml.MappingNode} + + // url field + objNode.Content = append(objNode.Content, + &yaml.Node{Kind: yaml.ScalarNode, Value: "url"}, + &yaml.Node{Kind: yaml.ScalarNode, Value: dep.URL}, + ) + + // select field + selKey := &yaml.Node{Kind: yaml.ScalarNode, Value: "select"} + selSeq := &yaml.Node{Kind: yaml.SequenceNode} + for _, s := range dep.Select { + selSeq.Content = append(selSeq.Content, + &yaml.Node{Kind: yaml.ScalarNode, Value: s}, + ) + } + objNode.Content = append(objNode.Content, selKey, selSeq) + + mapNode.Content = append(mapNode.Content, aliasNode, objNode) + } + } + + mapping.Content = append(mapping.Content, keyNode, mapNode) +} +``` + +### validate.go Changes (lines 54–59) + +```go +// BEFORE: +for alias, url := range m.Dependencies { + if !depURLPattern.MatchString(url) { + errs = append(errs, fmt.Errorf("dependencies[%q]: %q does not match ...", alias, url)) + } +} + +// AFTER: +for alias, dep := range m.Dependencies { + if !depURLPattern.MatchString(dep.URL) { + errs = append(errs, fmt.Errorf("dependencies[%q]: %q does not match ...", alias, dep.URL)) + } + for _, sel := range dep.Select { + if strings.HasPrefix(sel, "/") { + errs = append(errs, fmt.Errorf("dependencies[%q].select: %q must be a relative path (no leading '/')", alias, sel)) + } + if strings.Contains(sel, "..") { + errs = append(errs, fmt.Errorf("dependencies[%q].select: %q must not contain '..' (path traversal)", alias, sel)) + } + } +} +``` + +This requires adding `"strings"` to validate.go imports (currently only `"fmt"` and `"regexp"`). + +--- + +## 3. Dependencies Consumer Audit + +Every file:line that references `m.Dependencies` or uses the dependency value as a string. Each must be updated for the `map[string]string` → `map[string]DependencySpec` change. + +### internal/manifest/types.go:23 +```go +// BEFORE: Dependencies map[string]string `yaml:"dependencies,omitempty"` +// AFTER: Dependencies map[string]DependencySpec `yaml:"dependencies,omitempty"` +``` + +### internal/manifest/validate.go:55–58 +```go +// BEFORE: for alias, url := range m.Dependencies { ... url ... } +// AFTER: for alias, dep := range m.Dependencies { ... dep.URL ... } +``` +Plus new select path validation loop (see §2 above). + +### internal/manifest/write.go:31–33 +```go +// BEFORE: addStringMap(mapping, "dependencies", m.Dependencies) +// AFTER: addDependencies(mapping, "dependencies", m.Dependencies) +``` + +### internal/manifest/write_test.go:17–19 +```go +// BEFORE: Dependencies: map[string]string{"dep-a": "github.com/org/repo-a@v1.0.0"}, +// AFTER: Dependencies: map[string]DependencySpec{"dep-a": {URL: "github.com/org/repo-a@v1.0.0"}}, +``` +Also line 55–56: `parsed.Dependencies["dep-a"]` → `parsed.Dependencies["dep-a"].URL` + +### internal/manifest/write_test.go:127–131 (TestWriteMapKeyOrder) +```go +// BEFORE: +Dependencies: map[string]string{ + "charlie": "github.com/org/charlie@v1.0.0", + "alpha": "github.com/org/alpha@v1.0.0", + "bravo": "github.com/org/bravo@v1.0.0", +}, +// AFTER: +Dependencies: map[string]DependencySpec{ + "charlie": {URL: "github.com/org/charlie@v1.0.0"}, + "alpha": {URL: "github.com/org/alpha@v1.0.0"}, + "bravo": {URL: "github.com/org/bravo@v1.0.0"}, +}, +``` + +### internal/manifest/parse_test.go:44–48 +```go +// BEFORE: +if m.Dependencies["git-ops"] != "github.com/example/git@v1.0.0" { + t.Errorf("Dependencies[git-ops] = %q", m.Dependencies["git-ops"]) +} +// AFTER: +if m.Dependencies["git-ops"].URL != "github.com/example/git@v1.0.0" { + t.Errorf("Dependencies[git-ops].URL = %q", m.Dependencies["git-ops"].URL) +} +``` + +### internal/manifest/parse_test.go:72–73 +```go +// len(m.Dependencies) — no change needed (len works on any map) +``` + +### internal/manifest/validate_test.go:25–27 +```go +// BEFORE: Dependencies: map[string]string{"git-ops": "github.com/example/git@v1.0.0"}, +// AFTER: Dependencies: map[string]DependencySpec{"git-ops": {URL: "github.com/example/git@v1.0.0"}}, +``` +Same pattern for lines 133, 207, 221, 234. + +### internal/resolve/resolver.go:56 +```go +// len(m.Dependencies) == 0 — no change needed (len works on any map) +``` + +### internal/resolve/resolver.go:205 +```go +// len(depManifest.Dependencies) > 0 — no change needed +``` + +### internal/resolve/resolver.go:282–296 +```go +// BEFORE: +for alias, depURL := range m.Dependencies { + parsed, err := ParseDepURL(depURL) + ... + dep := ResolvedDep{ + URL: depURL, + Alias: alias, + Source: source, + RefType: parsed.RefType, + } +// AFTER: +for alias, depSpec := range m.Dependencies { + parsed, err := ParseDepURL(depSpec.URL) + ... + dep := ResolvedDep{ + URL: depSpec.URL, + Alias: alias, + Source: source, + RefType: parsed.RefType, + Select: depSpec.Select, // NEW field on ResolvedDep + } +``` + +### internal/resolve/resolver.go:327 +```go +// len(depManifest.Dependencies) > 0 — no change needed +``` + +### internal/cli/add.go:85–88 +```go +// BEFORE: +if existing, ok := m.Dependencies[alias]; ok { + isUpdate = true + if existing == depURL { ... } + cmd.Printf("Updating %q: %s → %s\n", alias, existing, depURL) +} +// AFTER: +if existing, ok := m.Dependencies[alias]; ok { + isUpdate = true + if existing.URL == depURL { ... } + cmd.Printf("Updating %q: %s → %s\n", alias, existing.URL, depURL) +} +``` + +### internal/cli/add.go:95–98 +```go +// BEFORE: +if m.Dependencies == nil { + m.Dependencies = make(map[string]string) +} +m.Dependencies[alias] = depURL +// AFTER: +if m.Dependencies == nil { + m.Dependencies = make(map[string]manifest.DependencySpec) +} +m.Dependencies[alias] = manifest.DependencySpec{URL: depURL} +``` + +### internal/cli/get.go:109 +```go +// BEFORE: Dependencies: make(map[string]string), +// AFTER: Dependencies: make(map[string]manifest.DependencySpec), +``` + +### internal/cli/get.go:113–114 +```go +// BEFORE: m.Dependencies = make(map[string]string) +// AFTER: m.Dependencies = make(map[string]manifest.DependencySpec) +``` + +### internal/cli/get.go:120–124 +```go +// BEFORE: +existing, ok := m.Dependencies[dep.alias] +if !ok { continue } +if existing == dep.url { ... } +// AFTER: +existingSpec, ok := m.Dependencies[dep.alias] +if !ok { continue } +if existingSpec.URL == dep.url { ... } +``` + +And line 134: `existingParsed, existingErr := resolve.ParseDepURL(existing)` → `resolve.ParseDepURL(existingSpec.URL)` +And line 148: `m.Dependencies[newAlias]` — just checking existence, no change needed. + +### internal/cli/get.go:196 +```go +// BEFORE: m.Dependencies[dep.alias] = dep.url +// AFTER: m.Dependencies[dep.alias] = manifest.DependencySpec{URL: dep.url} +``` + +### internal/cli/update.go:76 +```go +// len(m.Dependencies) == 0 — no change needed +``` + +### internal/cli/update.go:91–93 +```go +// BEFORE: +if _, ok := m.Dependencies[targetAlias]; !ok { + return fmt.Errorf("... %s", targetAlias, availableAliases(m.Dependencies)) +// AFTER: Need to change availableAliases signature (see below) +``` + +### internal/cli/update.go:106 +```go +// BEFORE: for alias, depURL := range m.Dependencies { +// AFTER: for alias, depSpec := range m.Dependencies { +// depURL := depSpec.URL +``` +Lines 111, 143, 144, 155, 161 use `depURL` — all will work via the local `depURL := depSpec.URL`. + +### internal/cli/update.go:161 +```go +// BEFORE: m.Dependencies[alias] = newURL +// AFTER: m.Dependencies[alias] = manifest.DependencySpec{URL: newURL, Select: depSpec.Select} +``` +**Critical**: Must preserve the existing `Select` when updating the URL. + +### internal/cli/update.go:180 +```go +// BEFORE: for alias, depURL := range m.Dependencies { +// AFTER: for alias, depSpec := range m.Dependencies { +// depURL := depSpec.URL +``` + +### internal/cli/remove.go:69–71 +```go +// BEFORE: +depURL, ok := m.Dependencies[alias] +if !ok { + available := availableAliases(m.Dependencies) +// AFTER: +depSpec, ok := m.Dependencies[alias] +if !ok { + available := availableAliases(m.Dependencies) +// ... use depSpec.URL where depURL was used (lines 81, 94, 130) +``` + +### internal/cli/remove.go:87 +```go +// delete(m.Dependencies, alias) — no change needed (works on any map) +``` + +### internal/cli/remove.go:207 +```go +// BEFORE: func availableAliases(deps map[string]string) string { +// AFTER: func availableAliases(deps map[string]manifest.DependencySpec) string { +``` + +### internal/cli/list.go:44 +```go +// BEFORE: for alias, depURL := range m.Dependencies { +// AFTER: for alias, depSpec := range m.Dependencies { +// depURL := depSpec.URL (used on line 45) +``` +Actually line 45 does `parsed, err := resolve.ParseDepURL(depURL)` which becomes `resolve.ParseDepURL(depSpec.URL)`. + +### internal/cli/tree.go:39 +```go +// BEFORE: for alias, depURL := range m.Dependencies { +// AFTER: for alias, depSpec := range m.Dependencies { +// depURL := depSpec.URL +``` + +### internal/cli/outdated.go:29, 34, 64–65, 71 +```go +// len(m.Dependencies) — no change +// for alias := range m.Dependencies — no change (iterating keys only) +// Line 71: depURL := m.Dependencies[alias] → depSpec := m.Dependencies[alias]; depURL := depSpec.URL +``` + +### internal/cli/install.go:70, 171 +```go +// len(m.Dependencies) == 0 — no change needed +``` + +### internal/validate/runner.go:258 +```go +// len(m.Dependencies) == 0 — no change needed +``` + +### internal/validate/runner.go:267–268 +```go +// BEFORE: +for _, url := range m.Dependencies { + depURLs[url] = true + if _, ok := p.Resolved[url]; !ok { +// AFTER: +for _, dep := range m.Dependencies { + depURLs[dep.URL] = true + if _, ok := p.Resolved[dep.URL]; !ok { +``` + +### internal/validate/runner.go:318 +```go +// BEFORE: for alias, url := range m.Dependencies { +// AFTER: for alias, dep := range m.Dependencies { +// url := dep.URL +``` + +### Test files with map[string]string literal for Dependencies + +Files that construct `Manifest` literals with `Dependencies: map[string]string{...}`: +- `internal/manifest/write_test.go:17` → `map[string]DependencySpec` +- `internal/manifest/write_test.go:127` → `map[string]DependencySpec` +- `internal/manifest/validate_test.go:25,133,207,221,234` → `map[string]DependencySpec` +- `internal/cli/remove_test.go:246` → `map[string]DependencySpec` (via grep) +- `internal/cli/update_test.go:106` → `map[string]DependencySpec` (via grep) +- Any other test constructing `manifest.Manifest` with Dependencies + +**Total: ~45 individual line-level changes across 14 files.** + +--- + +## 4. Resolver Filter Implementation + +### ResolvedDep Struct Change (types.go) + +```go +type ResolvedDep struct { + URL string + Alias string + Commit string + Integrity string + Skills []string + SkillPaths []string + Source string + RefType RefType + Select []string // NEW: subpath filter from manifest (empty = all) +} +``` + +### Exact `filterBySelect` Function + +New function in `internal/resolve/resolver.go`: + +```go +// filterBySelect filters discovered skills to only those matching the select +// paths. Returns an error if any select path has no matching skill. +func filterBySelect(names, dirs []string, files map[string][]byte, selectPaths []string) ([]string, []string, map[string][]byte, error) { + if len(selectPaths) == 0 { + return names, dirs, files, nil + } + + // Normalize select paths for comparison + normalized := make([]string, len(selectPaths)) + for i, s := range selectPaths { + s = strings.TrimPrefix(s, "./") + s = strings.TrimSuffix(s, "/") + normalized[i] = s + } + + // Build lookup from dir → index for matching + dirIndex := make(map[string]int, len(dirs)) + for i, d := range dirs { + dirIndex[d] = i + } + + var filteredNames []string + var filteredDirs []string + filteredFiles := make(map[string][]byte) + + for _, sel := range normalized { + idx, ok := dirIndex[sel] + if !ok { + return nil, nil, nil, fmt.Errorf("selected path %q does not match any skill in the package (available: %s)", + sel, strings.Join(dirs, ", ")) + } + filteredNames = append(filteredNames, names[idx]) + filteredDirs = append(filteredDirs, dirs[idx]) + + // Include files under this skill directory + prefix := sel + "/" + if sel == "" { + prefix = "" + } + for path, content := range files { + if prefix == "" || strings.HasPrefix(path, prefix) { + filteredFiles[path] = content + } + } + } + + return filteredNames, filteredDirs, filteredFiles, nil +} +``` + +### Where in `resolveOne()` the Filter is Called (resolver.go:367–377) + +```go +// BEFORE (lines 367–377): +skillNames, skillPaths, skillFiles, err := r.discoverSkillsForDep(cloneURL, commitSHA) +if err != nil { + return dep, err +} +dep.Skills = skillNames +dep.SkillPaths = skillPaths +dep.Integrity = integrity.Digest(skillFiles) + +// AFTER: +skillNames, skillPaths, skillFiles, err := r.discoverSkillsForDep(cloneURL, commitSHA) +if err != nil { + return dep, err +} + +// Apply select filter if specified +if len(dep.Select) > 0 { + skillNames, skillPaths, skillFiles, err = filterBySelect(skillNames, skillPaths, skillFiles, dep.Select) + if err != nil { + return dep, fmt.Errorf("filtering skills for %s: %w", dep.URL, err) + } +} + +dep.Skills = skillNames +dep.SkillPaths = skillPaths +dep.Integrity = integrity.Digest(skillFiles) +``` + +### How `collectDeps()` Passes Select Through (resolver.go:282–297) + +Already covered in §3 above. The `depSpec.Select` is assigned to `dep.Select` when constructing the `ResolvedDep` in `collectDeps()`: + +```go +dep := ResolvedDep{ + URL: depSpec.URL, + Alias: alias, + Source: source, + RefType: parsed.RefType, + Select: depSpec.Select, // threads select through to resolveOne +} +``` + +For **transitive** dependencies (lines 321–333), the `collectDeps` recurse uses the _dependency's own manifest_ which won't have select — transitive deps are resolved in full per spec assumption. + +### MVS Select-Merge Logic (resolver.go:80–166) + +When MVS groups by `PackageIdentity()` and multiple manifest entries point to the same package with different selects, the selects must be **unioned**. If any entry has an empty select (= all skills), the merged result is "all skills". + +Insert after MVS version selection (after line 123 for tags, line 135 for commits, line 147 for branches), before `selected[identity] = best`: + +```go +// Merge Select lists from all entries referencing this package. +// Empty select in any entry → all skills (nil). +var mergedSelect []string +allSkills := false +for _, dep := range deps { + if len(dep.Select) == 0 { + allSkills = true + break + } + mergedSelect = append(mergedSelect, dep.Select...) +} +if allSkills { + best.Select = nil +} else { + best.Select = deduplicateStrings(mergedSelect) +} +``` + +New helper: +```go +func deduplicateStrings(ss []string) []string { + seen := make(map[string]bool, len(ss)) + var result []string + for _, s := range ss { + if !seen[s] { + seen[s] = true + result = append(result, s) + } + } + sort.Strings(result) + return result +} +``` + +This merge logic should be factored out and applied **once** after the switch statement (all three ref-type branches), not duplicated in each case. + +--- + +## 5. CLI Changes Audit + +### internal/cli/add.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 39 | `var alias, depURL string` | No change (local var name) | +| 48 | `parsed, err := resolve.ParseDepURL(depURL)` | No change | +| 85 | `if existing, ok := m.Dependencies[alias]; ok {` | Value is now `DependencySpec` | +| 87 | `if existing == depURL {` | `if existing.URL == depURL {` | +| 88 | `cmd.Printf("... %s — nothing ...", alias, depURL)` | No change | +| 91 | `cmd.Printf("... %s → %s\n", alias, existing, depURL)` | `existing.URL` | +| 95–96 | `m.Dependencies = make(map[string]string)` | `make(map[string]manifest.DependencySpec)` | +| 98 | `m.Dependencies[alias] = depURL` | `manifest.DependencySpec{URL: depURL}` | + +**Interactive preview flow for `craft add` (FR-010):** + +Insert between lines 98 and 100 (after adding dep to manifest, before resolving). The flow: + +1. After `resolve.ParseDepURL(depURL)` succeeds +2. Create a temporary resolver to discover skills in the package +3. If TTY and skill count > 1 and no `--all` flag: + - Present interactive multi-select + - User toggles skills on/off + - If user selects a subset → `m.Dependencies[alias] = manifest.DependencySpec{URL: depURL, Select: selectedPaths}` + - If user selects all → `m.Dependencies[alias] = manifest.DependencySpec{URL: depURL}` (simple form) +4. Non-TTY or `--all` → install all (existing behavior) + +New flag: +```go +var addAll bool +// In init(): +addCmd.Flags().BoolVar(&addAll, "all", false, "Install all skills without interactive selection") +``` + +Injection point (after line 98, before line 100): +```go +// Interactive skill preview (only for TTY + multi-skill packages) +if !addAll && term.IsTerminal(int(os.Stdin.Fd())) { + skills, err := previewSkills(depURL) + if err == nil && len(skills) > 1 { + selected, err := promptSkillSelection(cmd, skills) + if err != nil { + return err + } + if len(selected) < len(skills) { + m.Dependencies[alias] = manifest.DependencySpec{URL: depURL, Select: selected} + } + } +} +``` + +### internal/cli/get.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 71 | `parsed, err := resolve.ParseDepURL(args[1])` | Parse subpath from `parsed.Subpath` | +| 75 | `deps = append(deps, depEntry{alias: args[0], url: args[1], parsed: parsed, ...})` | Add `select` field | +| 79–83 | `parsed, err := resolve.ParseDepURL(arg)` | Extract `parsed.Subpath` into select | +| 109 | `Dependencies: make(map[string]string)` | `make(map[string]manifest.DependencySpec)` | +| 113–114 | `m.Dependencies = make(map[string]string)` | `make(map[string]manifest.DependencySpec)` | +| 120–124 | `existing, ok := m.Dependencies[dep.alias]`; `existing == dep.url` | `existingSpec.URL == dep.url` | +| 134 | `resolve.ParseDepURL(existing)` | `resolve.ParseDepURL(existingSpec.URL)` | +| 196 | `m.Dependencies[dep.alias] = dep.url` | See below | + +**`#subpath` parsing and DependencySpec construction for `craft get`:** + +The `depEntry` struct (line 45) should gain a `selectPaths` field: +```go +type depEntry struct { + alias string + url string // URL without fragment + parsed *resolve.DepURL + explicitAlias bool + selectPaths []string // NEW: from #subpath parsing +} +``` + +After `ParseDepURL` succeeds (line 71 for explicit alias, line 79 for URL-only), extract the subpath: +```go +parsed, err := resolve.ParseDepURL(arg) +if err != nil { ... } + +// Extract subpath from parsed URL +var selectPaths []string +if parsed.Subpath != "" { + selectPaths = []string{parsed.Subpath} +} +// Use fragment-stripped URL for manifest storage +cleanURL := parsed.PackageIdentity() + "@" + parsed.RefString() + +deps = append(deps, depEntry{ + alias: parsed.Repo, + url: cleanURL, // without #fragment + parsed: parsed, + selectPaths: selectPaths, +}) +``` + +Line 196 changes: +```go +// BEFORE: m.Dependencies[dep.alias] = dep.url +// AFTER: +m.Dependencies[dep.alias] = manifest.DependencySpec{ + URL: dep.url, + Select: dep.selectPaths, +} +``` + +### internal/cli/update.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 91 | `if _, ok := m.Dependencies[targetAlias]; !ok {` | No change (existence check) | +| 93 | `availableAliases(m.Dependencies)` | Signature change (see §3) | +| 106 | `for alias, depURL := range m.Dependencies {` | `for alias, depSpec := range m.Dependencies { depURL := depSpec.URL` | +| 161 | `m.Dependencies[alias] = newURL` | `manifest.DependencySpec{URL: newURL, Select: depSpec.Select}` | +| 180 | `for alias, depURL := range m.Dependencies {` | `for alias, depSpec := range m.Dependencies { depURL := depSpec.URL` | + +**New-skill-discovery logic (FR-014):** + +After resolution succeeds (around line 195), for each selectively-installed dep, compare available skills vs selected skills: + +```go +// After resolution, check for new skills in selectively installed deps +for alias, depSpec := range m.Dependencies { + if len(depSpec.Select) == 0 { + continue // all skills installed, no discovery needed + } + for _, dep := range result.Resolved { + if dep.Alias != alias { + continue + } + // dep.Skills contains only selected skills. + // Need to discover ALL skills to compare. + // This requires a second discovery pass or caching all skills. + // See implementation notes below. + } +} +``` + +**Implementation approach for new-skill discovery:** The resolver's `resolveOne()` can store the full skill list before filtering in a new `AllSkillPaths` field on `ResolvedDep`, or we can re-discover after resolution. The cleaner approach is to add `AllSkillPaths []string` and `AllSkills []string` to `ResolvedDep`, populated before filtering, and use the diff in the update command. + +However, this adds complexity. **Recommended: defer to Phase 2** and implement as a separate pass in `runUpdate` that re-discovers skills for selectively installed deps. + +### internal/cli/remove.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 69 | `depURL, ok := m.Dependencies[alias]` | `depSpec, ok := m.Dependencies[alias]` | +| 70–71 | Uses `depURL` | Use `depSpec.URL` as `depURL` | +| 81 | `pf.Resolved[depURL]` | `pf.Resolved[depSpec.URL]` | +| 94 | `cmd.Printf("... %q (%s)\n", alias, depURL)` | `depSpec.URL` | +| 130 | `resolve.ParseDepURL(depURL)` | `resolve.ParseDepURL(depSpec.URL)` | +| 207 | `func availableAliases(deps map[string]string)` | `func availableAliases(deps map[string]manifest.DependencySpec)` | + +### internal/cli/list.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 44 | `for alias, depURL := range m.Dependencies {` | `for alias, depSpec := range m.Dependencies { depURL := depSpec.URL` | + +### internal/cli/tree.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 39 | `for alias, depURL := range m.Dependencies {` | `for alias, depSpec := range m.Dependencies { depURL := depSpec.URL` | + +### internal/cli/outdated.go — Per-Line Changes + +| Line | Current | Change | +|------|---------|--------| +| 71 | `depURL := m.Dependencies[alias]` | `depSpec := m.Dependencies[alias]; depURL := depSpec.URL` | + +### internal/cli/install.go — No Dep-Value Changes + +Lines 70 and 171 only check `len(m.Dependencies)` — no value access. No changes needed. + +### internal/cli/validate.go — No Direct Changes + +Calls `manifest.ValidateGlobal(m)` which internally uses the updated validate loop. No changes needed. + +--- + +## 6. Interactive UI Analysis + +### Existing UI Primitives (internal/ui/) + +**`progress.go`:** TTY-aware progress indicator with `Start`, `Update`, `Done`, `Fail`. Uses `\r\033[K` for line overwriting on TTY. Has `IsTTY()` method. Uses `golang.org/x/term` for detection. + +**`tree.go`:** Box-drawing tree renderer. `DepNode` struct with `Alias`, `URL`, `Skills`. `RenderTree()` writes to `io.Writer`. + +### Multi-Select Prompt: Does Not Exist — Must Be Built + +No existing multi-select prompt exists in `internal/ui/`. The codebase has two interactive prompts: +1. **Agent choice prompt** (`install.go:299–336`): Numbered list with single selection via `bufio.Scanner` +2. **Update confirmation** (`get.go:162–168`): Simple y/N prompt via `bufio.Scanner` + +Neither supports multi-select (toggle individual items on/off). + +### TTY Detection Pattern + +Already in use in two places: +- `get.go:118`: `isTTY := term.IsTerminal(int(os.Stdin.Fd()))` +- `ui/progress.go:25`: `term.IsTerminal(int(os.Stderr.Fd()))` + +The `golang.org/x/term` package is already a dependency (verified in `go.mod` via imports). + +### Recommended Multi-Select Implementation + +**Simple numbered-list approach** (no arrow-key navigation needed for MVP): + +```go +// internal/ui/select.go (new file) + +// MultiSelect presents a numbered list of items and lets the user +// toggle selections with space-separated numbers, then confirm. +// Returns indices of selected items. +func MultiSelect(w io.Writer, r io.Reader, prompt string, items []string) ([]int, error) { + // Print numbered list with [x] checkboxes + selected := make([]bool, len(items)) + for i := range selected { + selected[i] = true // all selected by default + } + + fmt.Fprintf(w, "\n%s\n\n", prompt) + for i, item := range items { + fmt.Fprintf(w, " %d) [x] %s\n", i+1, item) + } + fmt.Fprintf(w, "\nEnter numbers to toggle (space-separated), 'a' for all, 'n' for none, or Enter to confirm: ") + + scanner := bufio.NewScanner(r) + // ... toggle loop ... + // Returns selected indices +} +``` + +### How to Handle `--all` Flag + +Pattern from `install.go`: +```go +var addAll bool +addCmd.Flags().BoolVar(&addAll, "all", false, "Install all skills without interactive selection") +``` + +In `runAdd`: +```go +if addAll || !term.IsTerminal(int(os.Stdin.Fd())) { + // Skip interactive selection, install all +} else { + // Present multi-select +} +``` + +--- + +## 7. Testing Strategy + +### DepURL Parser Tests (depurl_test.go) + +- Add 7 new table-driven test cases (see §1 above) +- Add `TestDepURLSubpathMethods` for `String()`, `WithVersion()`, `PackageIdentity()` with subpath +- Verify `Raw` field stores fragment-stripped URL + +### Manifest Round-Trip Tests (write_test.go, parse_test.go) + +**New test: `TestWriteRoundTripStructuredDep`** +```go +func TestWriteRoundTripStructuredDep(t *testing.T) { + original := &Manifest{ + SchemaVersion: 1, + Name: "structured", + Skills: []string{"./skill"}, + Dependencies: map[string]DependencySpec{ + "simple": {URL: "github.com/org/a@v1.0.0"}, + "selective": {URL: "github.com/org/b@v2.0.0", Select: []string{"skills/docx", "skills/pdf"}}, + }, + } + var buf bytes.Buffer + if err := Write(original, &buf); err != nil { t.Fatalf(...) } + + // Verify YAML output format + output := buf.String() + // "simple" should be a scalar + if !strings.Contains(output, "simple: github.com/org/a@v1.0.0") { + t.Error("Simple dep should be written as scalar") + } + // "selective" should be a mapping + if !strings.Contains(output, "url: github.com/org/b@v2.0.0") { + t.Error("Structured dep should have url field") + } + + // Round-trip + parsed, err := Parse(&buf) + if err != nil { t.Fatalf(...) } + if parsed.Dependencies["simple"].URL != "github.com/org/a@v1.0.0" { + t.Error("Simple dep URL mismatch") + } + if len(parsed.Dependencies["simple"].Select) != 0 { + t.Error("Simple dep should have no Select") + } + if parsed.Dependencies["selective"].URL != "github.com/org/b@v2.0.0" { + t.Error("Structured dep URL mismatch") + } + if len(parsed.Dependencies["selective"].Select) != 2 { + t.Error("Structured dep should have 2 Select paths") + } +} +``` + +**New test: `TestParseStringAndObjectDeps`** +```go +func TestParseStringAndObjectDeps(t *testing.T) { + input := `schema_version: 1 +name: mixed-deps +skills: + - ./skill +dependencies: + simple: github.com/org/a@v1.0.0 + selective: + url: github.com/org/b@v2.0.0 + select: + - skills/docx + - skills/pdf +` + m, err := Parse(strings.NewReader(input)) + if err != nil { t.Fatalf("Parse failed: %v", err) } + + if m.Dependencies["simple"].URL != "github.com/org/a@v1.0.0" { ... } + if len(m.Dependencies["simple"].Select) != 0 { ... } + if m.Dependencies["selective"].URL != "github.com/org/b@v2.0.0" { ... } + if len(m.Dependencies["selective"].Select) != 2 { ... } +} +``` + +### Validation Tests (validate_test.go) + +**New test: `TestValidateSelectPaths`** +```go +func TestValidateSelectPaths(t *testing.T) { + tests := []struct { + name string + sel []string + wantErr bool + }{ + {"valid relative paths", []string{"skills/docx", "skills/pdf"}, false}, + {"absolute path", []string{"/skills/docx"}, true}, + {"path traversal", []string{"skills/../secrets"}, true}, + {"empty select", []string{}, false}, + {"leading dot-slash (normalized)", []string{"./skills/docx"}, false}, // depends on normalization in validate + } + for _, tc := range tests { + m := &Manifest{ + SchemaVersion: 1, + Name: "test", + Skills: []string{"./skill"}, + Dependencies: map[string]DependencySpec{"dep": {URL: "github.com/org/repo@v1.0.0", Select: tc.sel}}, + } + errs := Validate(m) + hasSelectErr := false + for _, e := range errs { + if strings.Contains(e.Error(), "select") { + hasSelectErr = true + } + } + if tc.wantErr && !hasSelectErr { t.Errorf(...) } + if !tc.wantErr && hasSelectErr { t.Errorf(...) } + } +} +``` + +### Resolver Filter Tests + +**New test: `TestFilterBySelect`** in `resolver_test.go`: +```go +func TestFilterBySelect(t *testing.T) { + names := []string{"docx", "pdf", "xlsx"} + dirs := []string{"skills/docx", "skills/pdf", "skills/xlsx"} + files := map[string][]byte{ + "skills/docx/SKILL.md": []byte("docx"), + "skills/pdf/SKILL.md": []byte("pdf"), + "skills/xlsx/SKILL.md": []byte("xlsx"), + } + + // Select 2 of 3 + n, d, f, err := filterBySelect(names, dirs, files, []string{"skills/docx", "skills/pdf"}) + if err != nil { t.Fatalf(...) } + if len(n) != 2 { t.Errorf("expected 2 names, got %d", len(n)) } + if len(d) != 2 { t.Errorf("expected 2 dirs, got %d", len(d)) } + // Verify files only include docx and pdf + + // Select non-existent path + _, _, _, err = filterBySelect(names, dirs, files, []string{"skills/nonexistent"}) + if err == nil { t.Error("expected error for non-existent path") } + + // Empty select = all + n, d, f, err = filterBySelect(names, dirs, files, nil) + if err != nil { t.Fatalf(...) } + if len(n) != 3 { t.Error("empty select should return all") } +} +``` + +### MVS Select Merge Tests + +Test the deduplication and "empty = all" merge logic in a resolver integration test. + +--- + +## 8. Recommended Phases + +### Phase 1: Core Type Changes and Parse/Write Round-Trip +**Goal:** Change the fundamental type and ensure all existing code compiles and tests pass. + +**Changes:** +1. Add `DependencySpec` struct to `types.go` with `UnmarshalYAML` +2. Change `Manifest.Dependencies` from `map[string]string` to `map[string]DependencySpec` +3. Add `addDependencies` to `write.go`, update `Write()` call +4. Update `validate.go` loop (`dep.URL` instead of `url`) +5. Add select path validation (absolute, traversal checks) +6. Mechanical update of ALL consumers (§3 above — ~45 line changes across 14 files) +7. Update ALL test files with new type literals +8. Add new parse/write round-trip tests for both string and structured deps + +**Verification:** `go build ./...` && `go test ./...` — all existing tests pass, plus new round-trip tests. + +**Testable independently:** Yes. This phase produces a codebase where structured deps can be declared but `Select` is ignored at resolution time. Existing behavior is identical. + +### Phase 2: DepURL Subpath Parsing +**Goal:** `ParseDepURL` accepts `#subpath` fragments. + +**Changes:** +1. Add `Subpath` field to `DepURL` struct +2. Add `normalizeSubpath()` helper +3. Modify `ParseDepURL` to split on `#` before `@` +4. Update `String()` to append `#subpath` +5. Add ~7 new test cases to `depurl_test.go` +6. Add `TestDepURLSubpathMethods` + +**Verification:** `go test ./internal/resolve/...` — all parser tests pass including new subpath cases. + +**Testable independently:** Yes. Subpath is parsed but not yet used by any consumer. + +### Phase 3: Resolver Filter and Select Merge +**Goal:** Resolution filters skills by `Select` and merges selects for same-package MVS. + +**Changes:** +1. Add `Select []string` to `ResolvedDep` struct +2. Implement `filterBySelect()` in resolver.go +3. Insert filter call in `resolveOne()` after skill discovery +4. Thread `depSpec.Select` through `collectDeps()` into `ResolvedDep.Select` +5. Implement select-merge in MVS loop with `deduplicateStrings()` +6. Add `TestFilterBySelect` unit tests +7. Add integration test for select-merge scenario + +**Verification:** `go test ./internal/resolve/...` — filter and merge tests pass. End-to-end: `craft install` with a structured dep selects correct skills. + +**Testable independently:** Yes. This is the core functional feature — selective installation works. + +### Phase 4: CLI `craft get` Subpath Support +**Goal:** `craft get url#subpath` installs a single skill. + +**Changes:** +1. Extract `parsed.Subpath` into `depEntry.selectPaths` in `runGet()` +2. Use fragment-stripped URL for manifest storage +3. Construct `DependencySpec{URL: cleanURL, Select: selectPaths}` on line 196 +4. Add CLI test for `craft get` with `#subpath` + +**Verification:** `go test ./internal/cli/...` — get command tests pass with subpath argument. + +### Phase 5: Interactive `craft add` Preview +**Goal:** `craft add` shows interactive skill selection for multi-skill packages. + +**Changes:** +1. Create `internal/ui/select.go` with `MultiSelect` function +2. Add `previewSkills()` helper in `add.go` (discovers skills without full resolution) +3. Add `--all` flag to `addCmd` +4. Insert interactive flow in `runAdd()` between dep addition and resolution +5. Non-TTY detection: `term.IsTerminal(int(os.Stdin.Fd()))` +6. Add tests for `MultiSelect` (mock stdin) +7. Add CLI tests for `--all` flag behavior + +**Verification:** Manual TTY testing + unit tests for `MultiSelect`. + +### Phase 6: `craft update` New-Skill Discovery +**Goal:** `craft update` informs users about newly available skills. + +**Changes:** +1. After resolution, for selectively installed deps, re-discover all available skills +2. Diff against selected skills to find new ones +3. Print informational message: `"ℹ New skills available in %s: %s (use 'craft add' to include)"` +4. Add test for new-skill notification output + +**Verification:** CLI test verifying notification message appears when upstream adds skills. + +### Phase Summary + +| Phase | Files Changed | LOC Est. | Risk | Independently Testable | +|-------|--------------|----------|------|----------------------| +| 1: Core Types | 14 files | ~150 | Medium (wide refactor) | ✅ | +| 2: DepURL Subpath | 2 files | ~60 | Low | ✅ | +| 3: Resolver Filter | 2 files | ~120 | Medium | ✅ | +| 4: CLI get#subpath | 1 file | ~40 | Low | ✅ | +| 5: Interactive add | 3 files | ~150 | Medium (UI) | ✅ | +| 6: Update discovery | 1 file | ~50 | Low | ✅ | + +**Total estimated: ~570 lines of code changes across 16 unique files.** + +Phase 1 is the riskiest because it touches 14 files mechanically. It should be done first and verified with `go build ./... && go test ./...` before proceeding. Phases 2–4 can be implemented in sequence. Phases 5–6 are additive features that can be done in parallel or deferred. diff --git a/.paw/work/feature-purl/Docs.md b/.paw/work/feature-purl/Docs.md new file mode 100644 index 0000000..95c0a20 --- /dev/null +++ b/.paw/work/feature-purl/Docs.md @@ -0,0 +1,41 @@ +# Docs: Subpath Skill Selection + +## Architecture + +The feature spans five components: + +1. **DependencySpec type** (`internal/manifest/`) — Dependencies are now `DependencySpec` values, not plain strings. A `DependencySpec` transparently represents either a simple URL string or a structured `{url, select}` object via a custom YAML unmarshaler on `*yaml.Node`. Serialization canonicalizes: empty `select` → simple string. + +2. **DepURL `#subpath` parsing** (`internal/resolve/`) — The URL parser extracts an optional `#fragment` after the version/ref component, storing it in a `Subpath` field. The `#` separator aligns with PURL (ECMA-427) fragment syntax. + +3. **Resolver filtering** (`internal/resolve/`) — After discovering all skills in a package, the resolver filters against the `Select` paths. Only matched skills are included in integrity computation and installation. Unmatched select paths cause resolution failure with a descriptive error. When multiple aliases reference the same package, their select lists are unioned; an empty select (meaning "all") wins over any specific selection. + +4. **Interactive preview in `craft add`** (`internal/cli/`) — When adding a multi-skill package in a TTY, `craft add` fetches available skills and presents a numbered list. Users enter comma-separated skill numbers (e.g. 1,3,5), 'a' for all, or Enter for all. `--all` or non-TTY skips the prompt. + +5. **New-skill discovery in `craft update`** (`internal/cli/`) — During update, the resolver compares the full set of available skills against the current selection. Newly available skills are reported as informational messages. + +## Key Design Decisions + +- **PURL-compatible `#fragment` syntax** — Uses `#` as the subpath separator (not `/` or `@`), aligning with ECMA-427 Section 5.6.7. This keeps future full PURL adoption a smooth migration rather than a redesign. + +- **Consumer selection overrides package exports (FR-006)** — `select` filters against all discoverable skills in the repo, not just the package's declared `skills` list. This gives consumers full control. + +- **Format canonicalization** — On write, a structured dependency with empty/absent `select` is serialized as a plain string. This keeps manifests clean and backward-compatible. + +- **Select merge semantics** — When multiple aliases point to the same package, selections are unioned. If any alias has an empty select (all skills), the merged result is "all" — the most permissive intent wins. + +- **No schema version bump** — The structured format is a compatible extension of schema version 1. Older craft versions will fail to parse structured deps with a clear YAML error, which is acceptable for a minor version feature. + +## Testing + +Test coverage added across the feature: + +| Area | Coverage | +|------|----------| +| DepURL parsing | `#subpath` extraction, edge cases (empty fragment, multiple `#`, special chars) | +| DependencySpec YAML | Round-trip marshal/unmarshal for string and structured formats, canonicalization | +| Manifest validation | Reject absolute paths, `..` traversal, accept normalized paths | +| Resolver filtering | Select matching, unmatched path errors, multi-alias merging, empty-select semantics | +| Integration (craft add) | Interactive selection mock, `--all` flag, non-TTY fallback | +| Integration (craft get) | `#subpath` single-skill install | +| Integration (craft update) | New-skill discovery messaging | diff --git a/.paw/work/feature-purl/ImplementationPlan.md b/.paw/work/feature-purl/ImplementationPlan.md new file mode 100644 index 0000000..7d45279 --- /dev/null +++ b/.paw/work/feature-purl/ImplementationPlan.md @@ -0,0 +1,253 @@ +# Subpath Skill Selection Implementation Plan + +## Overview + +Add subpath-based skill selection to craft so consumers can cherry-pick specific skills from large packages. The feature introduces a structured dependency format in `craft.yaml` (`url` + `select` list), extends `DepURL` with `#subpath` fragment support, and adds resolver-level skill filtering. Interactive skill preview during `craft add` and new-skill discovery during `craft update` round out the user experience. + +The implementation follows a bottom-up strategy: foundational type changes first (highest risk, widest impact), then parser, resolver, CLI commands, and finally interactive UI and update notifications. + +## Current State Analysis + +- **Dependencies are strings**: `Manifest.Dependencies` is `map[string]string` — values are DepURL strings. This type appears in 14 files (~45 individual references). +- **DepURL has no subpath**: The parser splits on `@` only; no `#` fragment handling exists. +- **All skills are installed**: `discoverSkillsForDep()` returns all exported skills. There is no consumer-side filter mechanism. +- **`craft add` has no preview**: Dependencies are added without showing what skills a package contains. +- **`craft update` has no awareness of selections**: It re-resolves but doesn't compare available vs. selected skill sets. + +Key constraints from research: +- The codebase uses `gopkg.in/yaml.v3` with `*yaml.Node`-based serialization for full output control (write.go). Custom unmarshalers via `*yaml.Node` parameter are well-supported. +- `DiscoverSkills()` in `discover.go` is already exported and reusable for skill preview. +- The fetcher's `ListTree` cache is per-instance and warms automatically across discovery and resolution calls. + +## Desired End State + +- A `craft.yaml` with structured dependencies (`url` + `select`) resolves and installs only the selected subset of skills +- `craft get url#subpath` installs a single skill from a multi-skill package +- `craft add` interactively presents available skills for selection in TTY environments +- `craft update` informs users about newly available skills in selectively installed packages +- All existing `craft.yaml` files continue to parse and function identically (backward compatible) +- All existing tests pass; new tests cover structured deps, subpath parsing, filtering, and merging + +**Verification approach**: Each phase is verified with `go build ./... && go test ./...`. The final state is verified end-to-end by creating a test `craft.yaml` with mixed string and structured deps, running `craft install`, and confirming only selected skills are present. + +## What We're NOT Doing + +- Full PURL adoption (`pkg:type/...` format) — deferred per WorkShaping decision +- Glob patterns in `select` (e.g., `skills/doc*`) — explicit paths only +- `exclude` field (inverse of select) — `select` only +- OCI registry support — deferred to PURL adoption +- `select` propagation to transitive dependencies — consumer `select` applies only to direct deps +- Percent-encoding in subpath fragments — skill paths are simple alphanumeric +- Schema version bump — structured format is a compatible extension of schema version 1 +- Changes to pinfile format beyond recording filtered skill sets — no new fields needed + +## Phase Status + +- [x] **Phase 1: Core Type System** - Introduce `DependencySpec`, migrate all consumers from `map[string]string` +- [x] **Phase 2: DepURL Subpath Parsing** - Add `#subpath` fragment support to the URL parser +- [x] **Phase 3: Resolver Filtering** - Filter discovered skills by `select` list, merge selects across MVS +- [x] **Phase 4: CLI `craft get` Subpath** - Parse `#subpath` from CLI argument, create structured dep +- [x] **Phase 5: Interactive `craft add` Preview** - Skill discovery, multi-select UI, `--all` flag +- [x] **Phase 6: `craft update` Discovery** - Detect and report newly available skills +- [x] **Phase 7: Documentation** - Docs.md, README updates, CHANGELOG entry + +## Phase Candidates + + + +--- + +## Phase 1: Core Type System + +**Objective**: Replace `map[string]string` with `map[string]DependencySpec` across the entire codebase. After this phase, structured deps can be declared in YAML but `Select` is not yet used by the resolver. All existing behavior is preserved. + +This is the highest-risk phase because it touches 14 files mechanically. It must be done first so subsequent phases can build on the new type. + +### Changes Required + +- **`internal/manifest/types.go`**: Define `DependencySpec` struct with `URL string` and `Select []string` fields. Implement `UnmarshalYAML(*yaml.Node)` to handle both scalar (string → `DependencySpec{URL: value}`) and mapping (object → full struct) YAML nodes. Change `Manifest.Dependencies` from `map[string]string` to `map[string]DependencySpec`. + +- **`internal/manifest/write.go`**: Replace `addStringMap()` call for dependencies with a new `addDependencies()` function. When `Select` is empty, write as scalar value (preserving clean output for simple deps). When `Select` is non-empty, write as mapping with `url` and `select` keys. Follow existing `yaml.Node` serialization patterns. + +- **`internal/manifest/validate.go`**: Update dependency iteration to use `dep.URL` instead of `url`. Add select path validation: reject paths with leading `/` (absolute), containing `..` (traversal), or empty strings. Add `"strings"` import. + +- **`internal/manifest/parse.go`**: No changes needed — the custom `UnmarshalYAML` on `DependencySpec` is called automatically by the YAML library. + +- **Mechanical consumer updates (~45 lines across 14 files)**: Every reference to `m.Dependencies` values as `string` must change to use `.URL`. Files: `resolver.go` (lines 282-296), `add.go`, `get.go`, `update.go`, `remove.go`, `list.go`, `tree.go`, `outdated.go`, `install.go`, `validate/runner.go`, and test files. The CodeResearch §3 documents each line precisely. **Critical note for `update.go:161`**: When updating a dependency URL (e.g., version bump), the existing `Select` must be preserved — construct `DependencySpec{URL: newURL, Select: depSpec.Select}`, not just replace the URL string. + +- **Test file updates**: All test files that construct `Manifest` literals with `Dependencies: map[string]string{...}` must change to `map[string]DependencySpec{...}`. Files: `write_test.go`, `parse_test.go`, `validate_test.go`, `remove_test.go`, `update_test.go`. Assertions on dependency values change from `m.Dependencies["x"]` to `m.Dependencies["x"].URL`. + +- **New tests**: Round-trip test for structured deps (parse YAML with `url` + `select`, write back, verify identical output). Round-trip test for mixed manifest (some string deps, some structured deps). Validation tests for invalid select paths. + +### Success Criteria + +#### Automated Verification +- [ ] `go build ./...` compiles cleanly +- [ ] `go test ./...` — all existing tests pass (with updated type literals) +- [ ] New round-trip tests pass for both string and structured dep formats +- [ ] New validation tests pass for select path edge cases + +#### Manual Verification +- [ ] Create a `craft.yaml` with a structured dep (`url` + `select`), verify it parses without error +- [ ] Verify `craft validate` accepts the new format +- [ ] Verify simple string deps in existing manifests are unaffected + +--- + +## Phase 2: DepURL Subpath Parsing + +**Objective**: Extend `ParseDepURL` to accept `#subpath` fragments. The `Subpath` field is parsed and stored but not yet used by any consumer. This is a low-risk, isolated change. + +### Changes Required + +- **`internal/resolve/depurl.go`**: Add `Subpath string` field to `DepURL` struct. In `ParseDepURL`, split on `#` before the existing `@` split — extract fragment, strip from raw string, continue with existing parsing. Add `normalizeSubpath()` helper: trim leading `./`, trim trailing `/`, reject empty-after-trim as no subpath. Update `String()` to append `#subpath` when present in the reconstructed form. + +- **`internal/resolve/depurl_test.go`**: Add table-driven test cases: URL with tag + subpath, URL with commit + subpath, URL with branch + subpath, URL with empty fragment (`url#` → no subpath), URL with nested subpath (`skills/nested/docx`). Test that `PackageIdentity()` excludes subpath. Test that `String()` reconstructs correctly with subpath. Test that `WithVersion()` drops subpath (version bumps produce clean URLs). + +### Success Criteria + +#### Automated Verification +- [ ] `go test ./internal/resolve/...` — all parser tests pass including new subpath cases +- [ ] Existing `ParseDepURL` tests still pass (no regression) + +#### Manual Verification +- [ ] Parse `github.com/acme/skills@v1.0.0#skills/docx` — verify `Subpath` is `skills/docx` and `PackageIdentity()` is `github.com/acme/skills` + +--- + +## Phase 3: Resolver Filtering + +**Objective**: Make the resolver filter discovered skills against `select` lists and merge selects when MVS groups multiple entries for the same package. After this phase, `craft install` with a structured dep containing `select` installs only the selected skills. + +### Changes Required + +- **`internal/resolve/types.go`**: Add `Select []string` and `AllSkillPaths []string` fields to `ResolvedDep` struct. Both are transient (not persisted in pinfile). `Select` flows through the resolution pipeline and is consumed by `resolveOne()` to filter results. `AllSkillPaths` records the full set of discovered skill paths before filtering, enabling Phase 6's new-skill detection without a second fetch. + +- **`internal/resolve/resolver.go`**: + - In `collectDeps()`: When iterating `m.Dependencies`, thread `depSpec.Select` into `ResolvedDep.Select`. + - In the MVS loop: After selecting the best version for a package identity, merge `Select` lists from all entries. If any entry has an empty `Select` (meaning "all"), the merged result is empty (all skills). Otherwise, deduplicate the union. + - In `resolveOne()`: After skill discovery returns `names, paths, files`, store the full discovered set as `dep.AllSkillPaths` (new field on `ResolvedDep`) before filtering. Then apply filtering if `dep.Select` is non-empty. Implement `filterBySelect()` — match select paths against `dep.SkillPaths` with normalization (strip `./` prefix), return filtered names/paths/files. If any select path matches no discovered skill, return an error identifying the invalid path. Note: `AllSkillPaths` is needed by Phase 6 for new-skill discovery without a second fetch pass. + - **Discovery scope (FR-006)**: When `dep.Select` is non-empty, discovery MUST scan all SKILL.md files in the repository tree (auto-discovery mode via `DiscoverSkills(ListTree)`), regardless of whether the dependency's `craft.yaml` declares a `skills` export list. This ensures the consumer's selection can override the package's exports. When `dep.Select` is empty, use the existing discovery path (manifest exports first, auto-discovery fallback) for backward compatibility. + - **Integrity (FR-015)**: The filtering happens before `integrity.Digest(skillFiles)` is called, so the digest is automatically computed over only the selected skill files. No additional integrity changes needed. + +- **`internal/resolve/resolver_test.go`**: Add tests: `TestFilterBySelect` (3 skills, select 1, verify output), `TestResolveSelectMerge` (two deps same package, different selects, verify union), `TestResolveSelectNotFound` (select path with no match → error), `TestResolveEmptySelect` (empty select → all skills), `TestResolveSelectOverridesExports` (dependency exports only `skills/public` but consumer selects `skills/internal` — verify `skills/internal` is discovered and installed). + +### Success Criteria + +#### Automated Verification +- [ ] `go test ./internal/resolve/...` — filter and merge tests pass +- [ ] `go test ./...` — full test suite passes + +#### Manual Verification +- [ ] Create a test `craft.yaml` with `select: [skills/docx]` for a package with 3 skills, run `craft install`, verify only `docx` is installed +- [ ] Verify pinfile contains only the selected skill in `skills` and `skill_paths` +- [ ] Verify integrity digest matches the selected skill content only + +--- + +## Phase 4: CLI `craft get` Subpath + +**Objective**: `craft get github.com/acme/skills@v1.0.0#skills/docx` installs only the `docx` skill globally. Low risk — builds directly on Phases 2 and 3. + +### Changes Required + +- **`internal/cli/get.go`**: After `ParseDepURL(arg)`, check `parsed.Subpath`. If non-empty, construct a `DependencySpec` with `URL` set to the fragment-stripped URL (`parsed.PackageIdentity() + "@" + parsed.RefString()`) and `Select` set to `[]string{parsed.Subpath}`. If empty, use the existing simple `DependencySpec{URL: arg}` path. + +- **`internal/cli/get_test.go`** (or add test cases): Test `craft get` with `#subpath` argument — verify the manifest entry is created with correct `Select` and only the targeted skill is installed. + +### Success Criteria + +#### Automated Verification +- [ ] `go test ./internal/cli/...` — get command tests pass with subpath +- [ ] `go test ./...` — full suite passes + +#### Manual Verification +- [ ] Run `craft get github.com/@v1.0.0#skills/docx` against a real or local test package, verify only `docx` is installed in the agent skills directory + +--- + +## Phase 5: Interactive `craft add` Preview + +**Objective**: When `craft add` targets a multi-skill package in a TTY environment, present available skills for interactive selection. `--all` flag and non-TTY environments skip the prompt. + +### Changes Required + +- **`internal/ui/select.go`** (new file): Implement a `MultiSelect` function that presents a numbered list of items with toggle controls. Accepts a list of skill names/paths, returns selected indices. Pattern: print numbered list, user enters numbers or `all`, confirm selection. Keep implementation simple — numbered input rather than arrow-key navigation. + +- **`internal/cli/add.go`**: + - Add `--all` flag to `addCmd` + - After adding the dependency to the manifest but before resolution, insert a preview flow: + 1. Create a fetcher instance (already done at line ~102) + 2. Use `DiscoverSkills()` from `resolve` package to discover available skills (requires `ListTree` + `ReadFiles` from fetcher) + 3. If multiple skills found AND TTY AND no `--all` flag: present `MultiSelect` + 4. If user selects a subset: set `DependencySpec.Select` to selected paths + 5. If user selects all or `--all` flag or non-TTY: leave `Select` empty (string format) + - TTY detection: use `term.IsTerminal(int(os.Stdin.Fd()))`, following pattern from `get.go` line 118 + +- **`internal/ui/select_test.go`** (new file): Unit tests for `MultiSelect` with mock stdin/stdout + +### Success Criteria + +#### Automated Verification +- [ ] `go build ./...` compiles (new UI package) +- [ ] `go test ./internal/ui/...` — multi-select tests pass +- [ ] `go test ./...` — full suite passes + +#### Manual Verification +- [ ] Run `craft add acme github.com/@v1.0.0` in a terminal — verify interactive skill list appears +- [ ] Select a subset — verify `craft.yaml` contains structured dep with `select` +- [ ] Select all — verify `craft.yaml` contains simple string dep +- [ ] Run with `--all` flag — verify no prompt appears, all skills installed +- [ ] Pipe input (non-TTY) — verify no prompt, all skills installed + +--- + +## Phase 6: `craft update` Discovery + +**Objective**: When updating a selectively installed dependency, inform users about newly available skills upstream. Inform only — no prompting, no auto-install. + +### Changes Required + +- **`internal/cli/update.go`**: After resolution completes for each dependency, check if the dependency has a `Select` list (from manifest). If so: + 1. Get the full set of discovered skills from `dep.AllSkillPaths` (populated by Phase 3's resolver changes) + 2. Compare against the selected skills in `dep.SkillPaths` + 3. If new skills exist that aren't in the selection, print an informational message: `"ℹ New skills available in %s: %s\n Use 'craft add' to include them"` + - No second fetch pass needed — `AllSkillPaths` was stored during resolution in Phase 3. + +- **`internal/cli/update_test.go`**: Add test verifying notification message appears when upstream has new skills. Add test verifying no message when skill set is unchanged. + +### Success Criteria + +#### Automated Verification +- [ ] `go test ./internal/cli/...` — update notification tests pass +- [ ] `go test ./...` — full suite passes + +#### Manual Verification +- [ ] Update a selectively installed dependency where upstream added a skill — verify informational message appears +- [ ] Update a dependency with no new skills — verify no extra output + +--- + +## Phase 7: Documentation + +**Objective**: Document the subpath skill selection feature for users and maintainers. + +### Changes Required + +- **`.paw/work/feature-purl/Docs.md`**: Technical reference for the implementation — architecture, key decisions, testing approach (load `paw-docs-guidance` for template) +- **`README.md`**: Add documentation for the structured dependency format, `select` field, `craft get #subpath` syntax, and interactive `craft add` behavior. Follow existing README structure and conventions. +- **`CHANGELOG`** (if project uses one): Entry for the subpath skill selection feature + +### Success Criteria + +- [ ] Documentation accurately describes the feature +- [ ] README examples are correct and follow existing style +- [ ] No broken links or references + +--- + +## References + +- Issue: https://github.com/agentskills/agentskills/discussions/210#discussioncomment-16284365 +- Spec: `.paw/work/feature-purl/Spec.md` +- Research: `.paw/work/feature-purl/SpecResearch.md`, `.paw/work/feature-purl/CodeResearch.md` diff --git a/.paw/work/feature-purl/Spec.md b/.paw/work/feature-purl/Spec.md new file mode 100644 index 0000000..c4f19dc --- /dev/null +++ b/.paw/work/feature-purl/Spec.md @@ -0,0 +1,180 @@ +# Feature Specification: Subpath Skill Selection + +**Branch**: feature/purl | **Created**: 2026-03-27 | **Status**: Draft +**Input Brief**: Enable consumers to cherry-pick individual skills from large packages instead of installing the entire exported set + +## Overview + +Craft currently operates on an all-or-nothing model for skill dependencies: when a consumer depends on a package, every exported skill in that package is resolved and installed. This works well for small, focused packages, but falls short for larger collections — monorepos with dozens of skills, curated skill libraries, or organizational skill bundles. A consumer who needs only a PDF-generation skill from a 20-skill productivity suite has no way to express that intent today. + +Subpath skill selection introduces a mechanism for consumers to declare exactly which skills they want from a dependency. In the manifest (`craft.yaml`), a dependency can now be either a simple URL string (existing behavior, installs all skills) or a structured object with a URL and a `select` list specifying skill directory paths. This preserves full backward compatibility while giving consumers precise control. + +The feature extends to the CLI as well. `craft get` gains fragment syntax (`#subpath`) for quick single-skill installs from the command line, and `craft add` presents an interactive preview of available skills when adding a multi-skill package, letting users choose which skills to include. The syntax for subpath fragments uses `#` — deliberately aligned with the PURL specification (ECMA-427) so that future PURL adoption remains a smooth migration path rather than a redesign. + +## Objectives + +- Enable consumers to install a specific subset of skills from any dependency, reducing unnecessary skill clutter in their agent environments +- Preserve full backward compatibility — existing `craft.yaml` files and workflows continue to work without modification +- Provide an interactive skill discovery experience during `craft add` so users can see what a package offers before deciding what to install +- Surface newly available skills during `craft update` so users stay informed without being nagged +- Align subpath syntax with PURL's `#subpath` convention (ECMA-427) to keep future PURL adoption viable + +## User Scenarios & Testing + +### User Story P1 – Selective Dependency Installation +Narrative: A developer maintains a workflow that depends on a large organizational skill package containing 15 skills. They only need 3 of them. They declare `select` in their `craft.yaml` and run `craft install`. Only the 3 selected skills are resolved, integrity-checked, and installed. + +Independent Test: Add a structured dependency with `select: [skills/docx, skills/pdf]` to `craft.yaml`, run `craft install`, and verify only `docx` and `pdf` skills are installed to the forge directory. + +Acceptance Scenarios: +1. Given a `craft.yaml` with a structured dependency selecting 2 of 5 available skills, When `craft install` is run, Then only the 2 selected skills are installed and the pinfile records only those skills +2. Given a `craft.yaml` with a simple string dependency (no select), When `craft install` is run, Then all exported skills are installed (unchanged behavior) +3. Given a structured dependency where a selected path does not match any skill in the package, When `craft install` is run, Then resolution fails with an error identifying the invalid path +4. Given a structured dependency with `select: []` (empty list), When `craft install` is run, Then all exported skills are installed (same as omitting select) + +### User Story P2 – Quick Single-Skill Install via CLI +Narrative: A user discovers a specific skill they want from a large package. They run `craft get` with a `#subpath` fragment to install just that one skill globally without creating a manifest. + +Independent Test: Run `craft get github.com/acme/skills@v1.0.0#skills/docx` and verify only the `docx` skill is installed to the agent's skill directory. + +Acceptance Scenarios: +1. Given a package URL with `#skills/docx` fragment, When `craft get ` is run, Then only the `docx` skill is installed globally +2. Given a package URL without a fragment, When `craft get ` is run, Then all exported skills are installed (unchanged behavior) +3. Given a `#subpath` that matches no skill in the package, When `craft get ` is run, Then the command fails with a clear error + +### User Story P3 – Interactive Skill Preview on Add +Narrative: A developer runs `craft add` for a new dependency. Before committing to the dependency, they see a list of all available skills and interactively choose which ones to include. The manifest is written with the appropriate format based on their selection. + +Independent Test: Run `craft add acme github.com/acme/skills@v1.0.0` in a TTY and verify an interactive skill list is presented for selection. + +Acceptance Scenarios: +1. Given a TTY terminal and a package with 5 skills, When `craft add` is run, Then an interactive skill list is displayed with selection controls +2. Given the user selects 2 of 5 skills in the interactive prompt, When the selection is confirmed, Then `craft.yaml` is written with a structured dependency containing `select: [path1, path2]` +3. Given the user selects all skills in the interactive prompt, When the selection is confirmed, Then `craft.yaml` is written with a simple string dependency (no select) +4. Given a non-TTY environment (CI/CD), When `craft add` is run, Then the interactive prompt is skipped and all skills are installed +5. Given the `--all` flag, When `craft add --all` is run, Then the interactive prompt is skipped and all skills are installed + +### User Story P4 – Discover New Skills on Update +Narrative: A developer has selectively installed 3 skills from a package. When they run `craft update`, the upstream version has added 2 new skills. The update succeeds for the selected skills and informs the user about the newly available ones. + +Independent Test: Run `craft update` on a dependency whose upstream has added new skills, and verify the output mentions the new skills with instructions on how to add them. + +Acceptance Scenarios: +1. Given a selectively installed dependency where upstream has added new skills, When `craft update` is run, Then selected skills are updated and new skills are listed as available +2. Given a selectively installed dependency where upstream has not added new skills, When `craft update` is run, Then the update proceeds normally with no additional output +3. Given a dependency installed without selection (all skills), When `craft update` is run, Then behavior is unchanged from current + +### User Story P5 – Multi-Alias Select Merging +Narrative: A team's `craft.yaml` has two aliases pointing to the same package with different selections (e.g., `docs` selects `docx, pdf` and `data` selects `xlsx`). Resolution merges the selections and produces a single pinfile entry. + +Independent Test: Create two manifest entries for the same package with different `select` lists, run `craft install`, and verify all selections are installed with one merged pinfile entry. + +Acceptance Scenarios: +1. Given two manifest entries for the same package with different select lists, When `craft install` is run, Then the union of all selected skills is installed +2. Given two manifest entries where one has select and the other has no select (all), When `craft install` is run, Then all skills are installed (empty select = all wins) + +### Edge Cases +- Structured dependency with `url` field but no `select` field: installs all skills (same as string format) +- Select path with leading `./` prefix: normalized to relative path (stripped) +- Select path with trailing `/`: normalized (stripped) +- Select path with `..` components: rejected by validation as path traversal +- Select path with leading `/`: rejected by validation as absolute path +- Package has no `craft.yaml` (auto-discovered skills): select filtering still works against discovered paths +- Select path matches a directory that exists but contains no `SKILL.md`: rejected with an error (same as non-matching path) +- Select/subpath overrides a package's own `skills` export list: if a consumer selects `skills/internal-tool` but the package only exports `skills/public-tool`, the consumer's selection wins — filtering applies against all discoverable skills in the repo, not just the package's declared exports +- Branch name containing `#` character: unsupported, documented limitation + +## Requirements + +### Functional Requirements + +- FR-001: The `DepURL` parser accepts an optional `#subpath` fragment after the version/ref component, storing it in a `Subpath` field (Stories: P2) +- FR-002: The manifest supports two dependency formats — a string (existing) and a structured object with `url` (required) and `select` (optional list of subpath strings) (Stories: P1, P3, P5) +- FR-003: Manifest parsing uses a custom YAML unmarshaler to transparently handle both string and object dependency formats (Stories: P1, P3) +- FR-004: Manifest validation rejects select paths that are absolute, contain `..`, or are otherwise invalid (Stories: P1) +- FR-005: Manifest serialization writes dependencies in canonical form — dependencies without a `select` list are written as simple strings, dependencies with a non-empty `select` list are written as structured objects. A structured dependency with empty or absent `select` is canonicalized to a simple string on write (Stories: P1, P3) +- FR-006: The resolver filters discovered skills against the `select` list, scanning all discoverable skills in the repository (not just the package's declared exports), returning only matching skills for integrity computation and installation (Stories: P1, P2) +- FR-007: The resolver fails with an error if any selected path does not match a discovered skill in the package (Stories: P1, P2) +- FR-008: When multiple manifest entries reference the same package identity, MVS selects one version and unions their `select` lists. If any entry has an empty select, the merged result is "all skills" (Stories: P5) +- FR-009: The pinfile records the filtered skill set (skills and skill paths) in a single merged entry per package identity (Stories: P1, P5) +- FR-010: `craft add` fetches available skills and presents an interactive selection prompt when in a TTY environment with a multi-skill package (Stories: P3) +- FR-011: `craft add` accepts an `--all` flag to skip interactive selection and install all skills (Stories: P3) +- FR-012: `craft add` auto-detects non-TTY environments and defaults to installing all skills without prompting (Stories: P3) +- FR-013: `craft get` parses `#subpath` from the URL argument and creates a structured dependency with the subpath as a single-element `select` list (Stories: P2) +- FR-014: `craft update` compares the full set of available skills against the selected set and informs the user about newly available skills (Stories: P4) +- FR-015: Integrity digests are computed over only the selected skill files, so the digest changes when the selection changes (Stories: P1) + +### Key Entities + +- **DependencySpec**: A manifest-level type representing a dependency as either a simple URL string or a structured object with `url` and `select` fields +- **Subpath**: An optional fragment component of a DepURL, representing a relative path within a package (e.g., `skills/docx`) +- **Select list**: An ordered list of subpath strings in a structured dependency that filters which skills are installed from a package + +### Cross-Cutting / Non-Functional + +- Backward compatibility: All existing `craft.yaml` files with string-only dependencies must parse and function identically to current behavior +- Schema version: No bump — the structured format is a compatible extension of schema version 1 +- Path normalization: Leading `./`, trailing `/` are stripped; paths are matched case-sensitively +- PURL alignment: The `#` fragment separator and relative subpath format align with ECMA-427 Section 5.6.7 + +## Success Criteria + +- SC-001: A structured dependency with `select` listing 2 of 5 available skills results in exactly 2 skills installed, with matching pinfile entries (FR-001, FR-002, FR-006, FR-009) +- SC-002: An existing `craft.yaml` with string-only dependencies produces identical resolution and installation results as before this change (FR-002, FR-003, FR-005) +- SC-003: A `craft get` command with `#subpath` installs exactly one skill from a multi-skill package (FR-001, FR-013) +- SC-004: Running `craft add` in a TTY on a package with 3+ skills presents an interactive selection prompt; selecting a subset writes a structured dependency with `select` (FR-010, FR-011) +- SC-005: A selected path that matches no skill in the package causes resolution to fail with a descriptive error (FR-007) +- SC-006: Two manifest entries selecting different skills from the same package produce a single pinfile entry with the union of selected skills (FR-008, FR-009) +- SC-007: `craft update` on a selectively installed dependency where upstream added a new skill outputs an informational message naming the new skill (FR-014) +- SC-008: Running `craft add` with `--all` or in a non-TTY environment skips the interactive prompt and installs all skills (FR-011, FR-012) + +## Assumptions + +- Skill directory paths within packages use simple alphanumeric names with hyphens and slashes — no percent-encoding support is needed for the initial implementation +- Git branch names containing `#` are extremely rare; the `#` fragment separator will not conflict with practical branch naming +- The interactive selection UI in `craft add` can be implemented with existing terminal UI patterns in the codebase (`internal/ui/`) or standard Go terminal libraries +- Transitive dependency skill selection is not needed — `select` applies only to direct dependencies; transitive dependencies are resolved in full per their own `craft.yaml` + +## Scope + +In Scope: +- `#subpath` fragment support in DepURL parser +- Structured dependency format (`url` + `select`) in `craft.yaml` +- Custom YAML unmarshaler for string-or-object dependency values +- Manifest validation for select paths +- Resolver-level skill filtering by select paths +- Select list merging for multi-alias same-package references +- Interactive skill preview/selection in `craft add` +- `--all` flag and non-TTY detection for `craft add` +- `#subpath` support in `craft get` +- Informational new-skill notification in `craft update` +- Pinfile recording of filtered skill sets + +Out of Scope: +- Full PURL adoption (`pkg:type/...` format) — deferred to future work +- Glob patterns in `select` (e.g., `skills/doc*`) — explicit paths only +- `exclude` field (inverse of select) — `select` only +- OCI registry support — deferred to PURL adoption +- `select` propagation to transitive dependencies +- Percent-encoding in subpath fragments +- Schema version bump + +## Dependencies + +- `gopkg.in/yaml.v3` custom `UnmarshalYAML` support via `*yaml.Node` (already in use) +- `golang.org/x/term` for TTY detection in `craft add` interactive mode (check if already a dependency) +- Existing `internal/resolve.DiscoverSkills()` function for skill preview in `craft add` + +## Risks & Mitigations + +- **Wide type change across codebase**: Changing `Dependencies map[string]string` to `map[string]DependencySpec` touches 10+ files. Mitigation: `DependencySpec.URL` is a drop-in replacement for the string value in all read paths; mechanical refactor. +- **Custom YAML unmarshaler edge cases**: YAML anchors, aliases, or unexpected node types interacting with the custom unmarshaler. Mitigation: Comprehensive test coverage for parse edge cases. +- **Select path matching ambiguity**: Different path normalizations (`./`, trailing `/`) could cause false mismatches. Mitigation: Normalize both select paths and discovered paths using the same function before comparison. +- **Interactive UI complexity**: Building a multi-select terminal UI. Mitigation: Start with simple numbered list + space-to-toggle pattern; enhance later if needed. + +## References + +- Issue: https://github.com/agentskills/agentskills/discussions/210#discussioncomment-16284365 +- PURL Specification: https://github.com/package-url/purl-spec (ECMA-427) +- Work Shaping: .paw/work/feature-purl/WorkShaping.md +- Research: .paw/work/feature-purl/SpecResearch.md diff --git a/.paw/work/feature-purl/SpecResearch.md b/.paw/work/feature-purl/SpecResearch.md new file mode 100644 index 0000000..f36c2f4 --- /dev/null +++ b/.paw/work/feature-purl/SpecResearch.md @@ -0,0 +1,658 @@ +# Spec Research: Subpath Skill Selection + +## Summary + +This document provides deep code-level analysis of every codebase component affected by the subpath skill selection feature. The feature adds `#subpath` fragment support to DepURL, introduces a `DependencySpec` type that can unmarshal from either a string or an object in craft.yaml, and extends the resolver to filter discovered skills by `select` lists. All existing formats remain backward-compatible. + +The codebase is well-structured with clear separation: `DepURL` handles parsing, `manifest` handles serialization, `resolve` handles discovery+MVS, `pinfile` records state, `install` handles file layout, and CLI commands orchestrate flows. Each layer has a clean injection point for the new subpath functionality. + +--- + +## DepURL Analysis + +### Current Structure (`internal/resolve/depurl.go`) + +The `DepURL` struct (lines 40–63) has 7 fields: + +```go +type DepURL struct { + Raw string // Original URL string + Host string // e.g., "github.com" + Org string // e.g., "example" + Repo string // e.g., "skills" + Version string // Semver without 'v' prefix (tag refs only) + Ref string // Commit SHA or branch name + RefType RefType // tag, commit, or branch +} +``` + +### Parsing Logic (`ParseDepURL`, lines 72–115) + +1. Splits on first `@` → `identity` (before) and `ref` (after) +2. Validates identity via `hostOrgRepoPattern` regex (line 25): `^([a-zA-Z0-9](...)?)/([a-zA-Z0-9_.-]+)/([a-zA-Z0-9_.-]+)$` +3. Classifies ref: + - `branch:` prefix → `RefTypeBranch` + - Matches `semverRefPattern` → `RefTypeTag` + - Hex string 7–64 chars → `RefTypeCommit` + - Otherwise → error + +### Changes Needed for Subpath + +**Add `Subpath` field to struct:** +```go +Subpath string // Optional subpath fragment (e.g., "skills/docx") +``` + +**Extend `ParseDepURL`:** +Before splitting on `@`, check for `#` fragment: +```go +// Before atIdx logic: +subpath := "" +if hashIdx := strings.Index(raw, "#"); hashIdx >= 0 { + subpath = raw[hashIdx+1:] + raw = raw[:hashIdx] // strip fragment for remaining parsing +} +``` + +Key considerations: +- Must parse `#` **before** `@` since the subpath is after `@ref`: `host/org/repo@v1.0.0#skills/docx` +- Actually, the order in the URL is: `identity@ref#subpath`. So split on `#` first from the full raw string, then proceed with existing `@` logic on the prefix. +- `Subpath` should be normalized: no leading `/`, no trailing `/`, no `./` prefix. +- Empty fragment (`url#`) should be treated as no subpath. + +**Affected methods:** +- `String()` (line 173): Must append `#subpath` if present. Currently returns `Raw` if set, or reconstructs. The `Raw` field will naturally include the fragment. But reconstructed form needs: `d.PackageIdentity() + "@" + d.RefString() + "#" + d.Subpath`. +- `WithVersion()` (line 167): Does NOT include subpath — it returns a bare URL for version comparison. No change needed. +- `PackageIdentity()` (line 120): Unchanged — subpath is not part of identity. +- `GitRef()` (line 126): Unchanged — subpath is not part of the git ref. + +**Impact on `depURLPattern` in `validate.go`:** +The manifest validation regex (line 15 of `validate.go`) will need to allow optional `#subpath` fragment OR validation needs to work with the `DependencySpec.URL` field which will NOT contain fragments (fragments only appear in `craft get` CLI args and in the `DependencySpec.Select` list). + +**Important architectural note:** Per WorkShaping, `#subpath` is used in two places: +1. `craft get github.com/acme/skills@v1.0.0#skills/docx` — CLI argument, single subpath via DepURL +2. `select: [skills/docx, skills/pdf]` — manifest structured dep, multiple subpaths in DependencySpec + +The DepURL `Subpath` field serves case 1. Case 2 uses `DependencySpec.Select`. They should not overlap — a DepURL in a manifest's `DependencySpec.URL` should NOT contain a `#fragment`. + +### Test Impact (`internal/resolve/depurl_test.go`) + +Current tests (lines 5–251) use table-driven pattern with `tests := []struct{ name, input string; want *DepURL; wantErr bool }`. New test cases needed: +- `"url with subpath"` → `github.com/acme/skills@v1.0.0#skills/docx` +- `"url with nested subpath"` → `github.com/acme/skills@v1.0.0#skills/nested/docx` +- `"url with empty fragment"` → `github.com/acme/skills@v1.0.0#` → subpath should be "" +- `"subpath with commit ref"` → `github.com/acme/tools@abc1234#tools/x` +- `"subpath with branch ref"` → `github.com/acme/tools@branch:main#tools/x` +- Error: `"subpath with leading slash"` → should normalize or reject + +--- + +## Manifest Analysis + +### Current Dependencies Type (`internal/manifest/types.go`) + +```go +Dependencies map[string]string `yaml:"dependencies,omitempty"` +``` + +This is a simple `map[string]string` where keys are aliases and values are DepURL strings. YAML unmarshal handles this natively. + +### Custom YAML Unmarshal Design + +The `Dependencies` field type must change to `map[string]DependencySpec`. The `DependencySpec` type needs a custom `UnmarshalYAML` to handle both forms: + +```yaml +# String form (backward compatible): +dependencies: + tools: github.com/example/tools@v1.0.0 + +# Object form (new): +dependencies: + acme: + url: github.com/acme/skills@v1.0.0 + select: + - skills/docx + - skills/pdf +``` + +**Proposed `DependencySpec` type:** +```go +type DependencySpec struct { + URL string // DepURL string (always present) + Select []string // Optional subpath filter list +} + +func (d *DependencySpec) UnmarshalYAML(value *yaml.Node) error { + if value.Kind == yaml.ScalarNode { + d.URL = value.Value + return nil + } + if value.Kind == yaml.MappingNode { + // Decode as struct + type raw struct { + URL string `yaml:"url"` + Select []string `yaml:"select"` + } + var r raw + if err := value.Decode(&r); err != nil { + return err + } + d.URL = r.URL + d.Select = r.Select + return nil + } + return fmt.Errorf("dependency must be a string or object, got %v", value.Kind) +} +``` + +The `gopkg.in/yaml.v3` library supports custom unmarshalers via `*yaml.Node` parameter. The codebase already uses `yaml.v3` (see `parse.go` line 8, `write.go` line 8). The `yaml.Node` approach gives access to `Kind` to distinguish scalar vs mapping nodes. + +### Parse Impact (`internal/manifest/parse.go`) + +`Parse()` (lines 13–25) uses `yaml.Unmarshal(data, &m)`. The custom `UnmarshalYAML` on `DependencySpec` will be called automatically — no changes needed in `parse.go` itself. The YAML library handles dispatching to the custom unmarshaler. + +### Validate Impact (`internal/manifest/validate.go`) + +The validation loop (lines 55–59) currently iterates `m.Dependencies` as `map[string]string`: +```go +for alias, url := range m.Dependencies { + if !depURLPattern.MatchString(url) { ... } +} +``` + +This changes to: +```go +for alias, dep := range m.Dependencies { + if !depURLPattern.MatchString(dep.URL) { ... } + // Validate select paths: no leading /, no .., no absolute paths + for _, sel := range dep.Select { + if strings.HasPrefix(sel, "/") || strings.Contains(sel, "..") { + errs = append(errs, ...) + } + } +} +``` + +The `depURLPattern` regex (line 15) should NOT be changed to allow `#` fragments — manifest URLs should never contain fragments. Subpaths go in the `select` list. + +### Write Impact (`internal/manifest/write.go`) + +`Write()` uses `addStringMap()` (line 31) for dependencies. This writes a flat `map[string]string`. For `DependencySpec`, we need a new serialization function: + +```go +func addDependencies(mapping *yaml.Node, key string, deps map[string]DependencySpec) { + // For each dep: + // - If Select is empty, write as scalar value (string) + // - If Select is non-empty, write as mapping with url + select +} +``` + +This preserves clean output: simple deps stay as `key: value`, structured deps get the object form. The `Write()` function already uses `yaml.Node` for full control over output structure (lines 16–46), so this fits naturally. + +### Backward Compatibility + +- Existing `craft.yaml` files with `dependencies: map[string]string` continue to parse correctly (the `UnmarshalYAML` scalar path handles them). +- `Write()` outputs simple deps as strings, so round-tripping preserves the format. +- The `Manifest` struct's YAML tag doesn't change: `yaml:"dependencies,omitempty"`. + +--- + +## Resolver Analysis + +### Skill Discovery Flow (`internal/resolve/resolver.go`) + +The key flow is in `resolveOne()` (lines 341–378): + +1. Parse DepURL +2. Check pinfile reuse (lines 348–357) +3. Resolve commit SHA via `fetcher.ResolveRef()` (line 361) +4. **Discover skills** via `discoverSkillsForDep()` (line 368) → returns `names, paths, files` +5. Set `dep.Skills`, `dep.SkillPaths`, compute `dep.Integrity` + +`discoverSkillsForDep()` (lines 382–399): +1. Try reading `craft.yaml` from the dep → if it has `Skills` list, use `discoverFromManifestSkills()` +2. Fallback: `autoDiscoverSkills()` scans for `SKILL.md` files + +### Where to Inject Subpath Filtering + +**Option A (recommended): Filter after discovery in `resolveOne()`.** + +After line 373 (`dep.Skills = skillNames`), add filtering: + +```go +if len(selectPaths) > 0 { + dep.Skills, dep.SkillPaths, skillFiles = filterBySelect( + dep.Skills, dep.SkillPaths, skillFiles, selectPaths, + ) +} +``` + +This keeps the discovery layer unchanged and adds a clean filter step. The `selectPaths` must be threaded through from the manifest's `DependencySpec.Select`. + +**Threading `select` to `resolveOne()`:** + +The `collectDeps()` function (lines 274–338) creates `ResolvedDep` structs from `m.Dependencies`. Currently line 282 iterates `alias, depURL` as strings. With `DependencySpec`, this becomes `alias, depSpec`, and the `ResolvedDep` would need a new field: + +```go +type ResolvedDep struct { + // ... existing fields ... + Select []string // Subpath filter from manifest (empty = all) +} +``` + +Or better: keep `ResolvedDep` focused on resolved state and pass `Select` via a separate map through `ResolveOptions`. + +**Practical approach:** Add `Select []string` to `ResolvedDep`. It's transient (not persisted in pinfile) but flows through the resolution pipeline. The resolver uses it in `resolveOne()` to filter. The pinfile records the **result** (filtered skills) not the filter itself. + +### MVS Implications + +MVS groups by `PackageIdentity()` (lines 82–89). When multiple manifest entries point to the same package: +- MVS selects one version (highest for tags) +- The `Select` lists from different entries should be **unioned** (per WorkShaping) + +In the MVS loop (lines 92–166), after selecting the best version, merge `Select` lists: +```go +// After selecting best: +var merged []string +for _, dep := range deps { + merged = append(merged, dep.Select...) +} +selected[identity].Select = dedupe(merged) +``` + +If any entry has empty `Select` (meaning "all"), the merged result should also be "all" (empty). + +### `collectDeps()` Changes + +Line 282: `for alias, depURL := range m.Dependencies` must change to handle `DependencySpec`: +```go +for alias, depSpec := range m.Dependencies { + parsed, err := ParseDepURL(depSpec.URL) + // ... + dep := ResolvedDep{ + URL: depSpec.URL, + Alias: alias, + Source: source, + RefType: parsed.RefType, + Select: depSpec.Select, + } +} +``` + +For transitive dependencies (parsing a dep's own `craft.yaml`), the `Select` from the *consumer's* manifest does NOT propagate — the transitive dep's own skills list is used unchanged. Only direct dependency `Select` applies. + +### `DiscoverSkills` in `discover.go` + +The standalone `DiscoverSkills()` function (lines 17–69) is used independently of the resolver. It returns `[]DiscoveredSkill`. This function doesn't need modification — filtering happens at a higher level. + +--- + +## Pinfile Analysis + +### Current Structure (`internal/pinfile/types.go`) + +```go +type Pinfile struct { + PinVersion int `yaml:"pin_version"` + Resolved map[string]ResolvedEntry `yaml:"resolved"` +} + +type ResolvedEntry struct { + Commit string `yaml:"commit"` + RefType string `yaml:"ref_type,omitempty"` + Integrity string `yaml:"integrity"` + Source string `yaml:"source,omitempty"` + Skills []string `yaml:"skills"` + SkillPaths []string `yaml:"skill_paths,omitempty"` +} +``` + +The pinfile key is the full DepURL string (e.g., `"github.com/acme/skills@v1.0.0"`). + +### Recording Selected Paths + +**Option A: Add `SelectedPaths` field to `ResolvedEntry`:** +```go +SelectedPaths []string `yaml:"selected_paths,omitempty"` +``` + +This records what the user selected, separate from `SkillPaths` (which records what was discovered and installed). Purpose: allows `craft update` to know which skills to re-check. + +**Option B (simpler): The `Skills` and `SkillPaths` already reflect the filtered set.** + +After filtering, `dep.Skills` and `dep.SkillPaths` contain only the selected skills. The pinfile already records these. So the pinfile naturally captures the filter result without needing the filter input. + +However, per WorkShaping, `craft update` should "inform about new skills." To do this, `craft update` needs to know the full available set vs. the selected set. This could be done by: +1. Re-discovering all skills at update time (fetch+scan) +2. Comparing against the pinfile's `Skills` list +3. Reporting any new skills not in the list + +This works without storing `SelectedPaths` in the pinfile — the manifest's `select` list is the source of truth for what was selected, and the pinfile records the result. + +### Merge Semantics + +Per WorkShaping: "One package = one pinfile entry. Selections from multiple aliases are unioned." + +The pinfile is keyed by DepURL (e.g., `github.com/acme/skills@v1.0.0`). If two manifest aliases both point to the same package (after MVS), they produce one pinfile entry with the union of selected skills. + +The current pinfile write logic (resolver.go lines 256–271) creates one entry per `ResolvedDep`. Since MVS already deduplicates to one entry per package identity, this naturally produces merged entries. The `Skills`/`SkillPaths` on the `ResolvedDep` would already be the union after MVS merging. + +### Integrity with Subpath Selection + +Currently, `integrity.Digest(skillFiles)` computes over ALL skill files. With subpath filtering, only selected skills' files are included. This means: +- The integrity digest changes when the selection changes (correct behavior — different content). +- If a user adds a new `select` entry, the pinfile integrity will change, triggering a re-resolve. + +--- + +## Installer Analysis + +### Composite Keys (`internal/install/installer.go`) + +**`Install()`** (lines 17–88): Takes `skills map[string]map[string][]byte` where keys are composite keys like `github.com/org/repo/skill-name`. Files are written to `target/compositeKey/`. + +**`CompositeKey()`** (line 119): `packageIdentity + "/" + skillName` — e.g., `"github.com/org/repo/my-skill"`. + +**`FlatKey()`** (line 96): Converts `/` to `--` for global installs: `"github.com--org--repo--my-skill"`. + +### Impact of Subpath on Naming + +**No impact.** The installer uses `skillName` (from `dep.Skills[i]`), not the subpath. The subpath is the *directory path within the repo* (e.g., `skills/docx`), while the skill name comes from `SKILL.md` frontmatter (e.g., `docx`). + +Example: For a skill at `skills/docx/SKILL.md` with `name: docx`: +- `CompositeKey("github.com/acme/big", "docx")` → `"github.com/acme/big/docx"` +- `FlatKey(...)` → `"github.com--acme--big--docx"` + +This is the same regardless of whether the skill was selected via subpath or installed as part of the full package. The installer doesn't know or care about selection. + +### `collectSkillFiles()` in `install.go` (lines 365–411) + +This function iterates `result.Resolved`, fetches skill files, and maps them by composite key. No changes needed — it already uses `dep.Skills[i]` and `dep.SkillPaths[i]` which will be the filtered set after subpath selection. + +--- + +## CLI Analysis + +### `craft add` Flow (`internal/cli/add.go`) + +Current flow (lines 38–187): +1. Parse args (alias + URL) +2. Validate URL via `ParseDepURL()` +3. Derive alias from repo name if not provided +4. Parse existing `craft.yaml` +5. Check for existing dependency (update vs. add) +6. Add to `m.Dependencies[alias] = depURL` +7. Resolve to verify +8. Write manifest atomically +9. Optionally install (`--install` flag) + +**Interactive preview injection point: Between steps 6 and 7.** + +After adding the dependency but before resolving, fetch the package and discover available skills: + +```go +// After line 98: m.Dependencies[alias] = depURL +// 1. Fetch package +// 2. Discover all skills (using existing discoverSkillsForDep or similar) +// 3. If multiple skills found AND stdin is TTY: +// - Present interactive selection UI +// - If user selects subset: write structured DependencySpec with select +// - If user selects all: write simple string format +// 4. If --all flag: skip interactivity, install all +``` + +**Flag design:** +- `--all` flag: skip interactive preview, install all skills (useful for CI/CD) +- Detect non-TTY (line 118 of `get.go` shows the pattern: `term.IsTerminal(int(os.Stdin.Fd()))`) + +**Writing structured deps:** +Currently line 98 does `m.Dependencies[alias] = depURL` (string). With `DependencySpec`: +```go +m.Dependencies[alias] = manifest.DependencySpec{ + URL: depURL, + Select: selectedPaths, // nil if all selected +} +``` + +### `craft get` Flow (`internal/cli/get.go`) + +Current flow (lines 43–307): +1. Parse args: detect alias vs URL pattern +2. Parse each URL via `ParseDepURL()` (lines 71, 79) +3. Load global manifest +4. Check for conflicts/existing deps +5. Add all deps to manifest +6. Resolve and install globally (flat keys) + +**Subpath support injection (lines 71–82):** + +Currently `ParseDepURL(args[1])` and `ParseDepURL(arg)`. With fragment support, `ParseDepURL` will extract `Subpath`. The flow needs: + +```go +parsed, err := resolve.ParseDepURL(arg) +if err != nil { ... } +// If subpath present, create structured dep +if parsed.Subpath != "" { + // Add as DependencySpec with Select: [parsed.Subpath] + m.Dependencies[alias] = manifest.DependencySpec{ + URL: parsed.PackageIdentity() + "@" + parsed.RefString(), // URL without fragment + Select: []string{parsed.Subpath}, + } +} else { + m.Dependencies[alias] = manifest.DependencySpec{URL: arg} +} +``` + +### `craft install` Flow (`internal/cli/install.go`) + +`runInstallProject()` (lines 150–253) and `runInstallGlobal()` (lines 50–148): +1. Parse manifest +2. Load existing pinfile +3. Resolve dependencies +4. Write pinfile +5. Collect skill files +6. Install + +**Minimal changes needed.** The install command reads `m.Dependencies` and passes the manifest to the resolver. If `Dependencies` is now `map[string]DependencySpec`, the resolver handles filtering internally. The install command doesn't need to know about `select` — it just installs whatever the resolver returns. + +The `collectSkillFiles()` helper (lines 365–411) uses `dep.Skills` and `dep.SkillPaths` from `ResolvedDep`, which are already filtered. + +--- + +## Fetcher Analysis + +### `ListTree()` (`internal/fetch/gogit.go`, lines 116–159) + +Returns all file paths in the repo tree at a given commit. Used for: +- Auto-discovery of skills (scanning for `SKILL.md`) +- Collecting skill directory files for integrity +- **New:** Previewing available skills in `craft add` + +Has an in-memory cache (`treeCache`, line 32) keyed by `url\x00commitSHA`. This means repeated `ListTree` calls for the same commit are free after the first call. + +### `ReadFiles()` (`internal/fetch/gogit.go`, lines 162–204) + +Reads specific files by path at a commit. Missing files are silently skipped. Size limit: 10MB per file. + +### For Interactive Preview + +To show available skills during `craft add`, the flow would be: +1. `fetcher.ResolveRef(cloneURL, parsed.GitRef())` → commitSHA +2. `fetcher.ListTree(cloneURL, commitSHA)` → all paths +3. Scan for `SKILL.md` files in paths +4. `fetcher.ReadFiles(cloneURL, commitSHA, mdPaths)` → read SKILL.md frontmatter +5. Parse frontmatter to get skill names +6. Present to user + +This is essentially what `discoverSkillsForDep()` does (resolver.go lines 382–399), but we need it before resolution (for the preview). Options: +- Extract discovery logic into a reusable function callable from CLI code +- Call `resolver.discoverSkillsForDep()` directly (it's currently unexported) +- Use the standalone `DiscoverSkills()` from `discover.go` (already exported) + +**Recommended:** Use `DiscoverSkills()` from `discover.go`. It takes `allPaths` and a `readFile` function — the CLI can construct these from the fetcher: + +```go +allPaths, _ := fetcher.ListTree(cloneURL, commitSHA) +readFile := func(path string) ([]byte, error) { + files, err := fetcher.ReadFiles(cloneURL, commitSHA, []string{path}) + if err != nil { return nil, err } + if content, ok := files[path]; ok { return content, nil } + return nil, fmt.Errorf("not found: %s", path) +} +skills, _ := resolve.DiscoverSkills(allPaths, readFile) +``` + +### Caching Implications + +The `ListTree` cache is per-fetcher-instance. In `craft add`, a new fetcher is created (line 102). The preview call and the subsequent resolve call will share the same fetcher instance, so the tree cache will be warm for the resolve step. No additional caching needed. + +--- + +## Testing Patterns + +### Conventions Observed + +1. **Table-driven tests** are the dominant pattern: + - `depurl_test.go`: `tests := []struct{ name, input string; want *DepURL; wantErr bool }` (lines 7–232) + - `validate_test.go`: Both inline and table-driven for name validation (lines 48–80) + - `manifest/validate_test.go`: Mix of individual test functions and table-driven for URL validation (lines 112–150) + +2. **MockFetcher** (`internal/fetch/mock.go`): Map-based mock with `Refs`, `Trees`, `Files`, `Errors` maps. Key format: `"url:ref"`, `"url:commit:path"`. + +3. **Helper functions**: `setupDep()` in `resolver_test.go` (lines 18–24) creates a standard dep setup. `assertContains()` in `validate_test.go` (lines 242–247). + +4. **No external test frameworks** — pure `testing` package with `t.Run()` subtests. + +5. **Error testing**: Check `wantErr bool`, then verify error message contents with `strings.Contains()`. + +6. **Round-trip testing**: `write_test.go` has `TestWriteRoundTrip` (lines 10–61) — serialize then parse, compare fields. + +7. **Determinism testing**: `TestWriteMapKeyOrder` (lines 122–167) writes 5 times, verifies identical output. + +### Test Strategy Recommendations + +**DepURL tests:** +- Add table entries for `#subpath` variants (with/without, various ref types) +- Test `String()` reconstruction includes subpath +- Test `PackageIdentity()` excludes subpath + +**Manifest tests:** +- Round-trip tests for both string and structured dep formats +- Custom YAML unmarshal tests: scalar → `DependencySpec{URL: "..."}`, mapping → full struct +- Write tests: structured deps serialize correctly, simple deps stay as strings +- Validation tests: invalid `select` paths (leading `/`, `..`, absolute) + +**Resolver tests:** +- New `TestResolveWithSelect`: mock dep with 3 skills, select 1, verify only 1 in result +- `TestResolveSelectMerge`: two deps same package, different selects, verify union +- `TestResolveSelectNotFound`: select path doesn't match any skill → error +- `TestResolveEmptySelect`: empty select list → all skills (same as omitting) + +**Pinfile tests:** +- Round-trip with selected subset of skills +- Verify integrity changes when selection changes + +**CLI tests (if integration-style):** +- `craft add` with `--all` flag +- `craft get` with `#subpath` URL + +--- + +## PURL Compatibility + +### PURL Subpath Syntax (ECMA-427, Section 5.6.7) + +The PURL specification defines subpath as: +``` +pkg:type/namespace/name@version?qualifiers#subpath +``` + +Key rules: +1. **Fragment separator**: `#` — same as URL fragment identifier per RFC 3986 +2. **Relative path**: Subpath is relative to the package root, no leading `/` +3. **Segments separated by `/`**: e.g., `#dir/subdir/file.txt` +4. **Percent-encoding**: Special characters must be percent-encoded per RFC 3986 +5. **No trailing `/`** in practice + +### Our Syntax Alignment + +Our proposed syntax: `github.com/acme/skills@v1.0.0#skills/docx` + +| PURL Rule | Our Implementation | Status | +|-----------|-------------------|--------| +| `#` fragment separator | ✅ Using `#` | Aligned | +| Relative path (no leading `/`) | ✅ `skills/docx` not `/skills/docx` | Aligned | +| `/`-separated segments | ✅ Standard path segments | Aligned | +| Percent-encoding | ⚠️ Not implemented yet | Low risk — skill paths are simple alphanumeric | + +**Difference from PURL:** Our URL format is not a full PURL (`pkg:type/...`). The `#subpath` syntax is borrowed from PURL but applied to our DepURL format (`host/org/repo@ref#subpath`). This is PURL-*compatible* syntax, not PURL itself. + +**Future PURL migration path:** If craft adopts full PURL (`pkg:agentskill/...`), the `#subpath` fragment semantics remain identical. The parser would change the prefix format but the subpath handling would transfer directly. + +--- + +## Risks and Concerns + +### 1. **`map[string]string` → `map[string]DependencySpec` is a Breaking Type Change** (Severity: High) + +Every file that reads `m.Dependencies` must be updated. A search for `m.Dependencies` usage across the codebase is critical: +- `resolver.go` lines 56, 205, 282 — iterate deps +- `add.go` lines 85, 95, 98 — check/set deps +- `get.go` lines 109, 113, 195 — check/set deps +- `install.go` lines 70, 171 — check emptiness +- `validate.go` lines 55–59 — validate URLs +- `write.go` line 31 — serialize deps +- `update.go`, `remove.go`, `list.go`, `tree.go` — various dep operations + +This is a wide-impact change, but it's straightforward since `DependencySpec.URL` replaces the string value in all read paths. + +### 2. **Custom YAML Unmarshaler Complexity** (Severity: Medium) + +The `UnmarshalYAML(*yaml.Node)` pattern is well-supported by `gopkg.in/yaml.v3` but adds complexity. Risks: +- Edge case: what if a YAML value is neither scalar nor mapping? → Return clear error +- Edge case: `select: []` (explicit empty list) vs. omitted `select` → Both should mean "all skills" +- YAML anchors/aliases interacting with custom unmarshaler → Should work but needs testing + +### 3. **Select Path Matching Ambiguity** (Severity: Medium) + +How to match `select: ["skills/docx"]` against discovered skills: +- Discovered skill at path `skills/docx` → exact match ✅ +- Discovered skill at path `./skills/docx` → need normalization (strip `./` prefix) +- Discovered skill at path `skills/docx/` → need normalization (strip trailing `/`) + +The resolver already normalizes with `strings.TrimPrefix(sp, "./")` (resolver.go line 405). Apply the same normalization to `select` paths. + +### 4. **Integrity Recalculation on Selection Change** (Severity: Low) + +If a user changes `select` (adds/removes a skill), the integrity digest changes. This means `craft install` will see a mismatch with the pinfile and require re-resolution. This is correct behavior but should be documented. + +### 5. **Transitive Dependency Select Propagation** (Severity: Low) + +Per WorkShaping, `select` only applies to direct dependencies. Transitive deps (from a dep's own `craft.yaml`) are always resolved in full. This is the correct design — a consumer shouldn't restrict what a package's own transitive dependencies expose. + +However, if Package A declares `select: [skills/x]` for Package B, and Package B's `craft.yaml` has transitive deps, those transitive deps still resolve fully. The `select` only filters B's skills, not B's dependencies. + +### 6. **`craft get` URL Parsing Order** (Severity: Low) + +When parsing `github.com/acme/skills@v1.0.0#skills/docx`: +- The `#` must be split before `@` processing +- But what about branch refs like `branch:feature/path#subpath`? The `/` in the branch name and the `/` in the subpath are unambiguous because `#` always separates them + +Potential issue: `@branch:name#with-hash` — the `#` is the fragment separator. Branch names with `#` would break. However, git branch names rarely contain `#`, and we can document this limitation. + +### 7. **Interactive UI in Non-TTY Environments** (Severity: Low) + +`craft add` in CI/CD pipelines (non-TTY) should either: +- Default to installing all skills (current behavior) +- Require `--all` flag explicitly +- Auto-detect non-TTY and skip interactive prompt + +The `get.go` already has TTY detection (`term.IsTerminal()`, line 118) that can be reused. + +### 8. **Schema Version Bump** (Severity: Decision needed) + +The structured dependency format is a schema extension. Should `schema_version` bump from 1 to 2? Arguments: +- **No bump:** The string format is still valid; older clients reading a manifest with only string deps work fine. Structured deps in a v1 manifest would fail gracefully on old clients (YAML parse would put an empty string or error). +- **Bump to 2:** Makes it explicit that the manifest uses new features. Old clients can give a clear "unsupported schema version" error. + +Recommendation: **Do NOT bump schema version.** The string format is backward-compatible, and older clients encountering structured deps will get a YAML parse error (which is informative enough). A bump would break all existing tooling unnecessarily. diff --git a/.paw/work/feature-purl/WorkShaping.md b/.paw/work/feature-purl/WorkShaping.md new file mode 100644 index 0000000..1715a8c --- /dev/null +++ b/.paw/work/feature-purl/WorkShaping.md @@ -0,0 +1,196 @@ +# Work Shaping: Subpath-Based Skill Selection (PURL-Informed) + +## Problem Statement + +Craft currently installs **all** exported skills from a dependency — consumers cannot cherry-pick individual skills from a large repo. When a package exports 12+ skills, users get everything whether they need it or not. + +This gap was surfaced by [travbale's comment on the agentskills RFC](https://github.com/agentskills/agentskills/discussions/210#discussioncomment-16284365), which proposed adopting [Package URLs (PURLs)](https://github.com/package-url/purl-spec) in craft. After exploration, the decision is to **not adopt PURL now** but to add subpath-based skill selection using PURL-compatible syntax, keeping PURL as a future option. + +**Who benefits:** Consumers depending on large skill packages (monorepos, curated collections) who only need a subset. + +## Decision: Why Not Full PURL (Yet) + +| Factor | Assessment | +|--------|------------| +| Skills only come from git repos today | PURL's multi-transport value (OCI, npm) isn't needed yet | +| DepURL is deeply embedded (16+ files, 19+ parse sites) | Full replacement is high-cost, low-immediate-value | +| PURL's `#subpath` solves the real problem | We can adopt just the subpath concept with `#` fragment syntax | +| No `pkg:agentskill/` type exists in PURL ecosystem | Ecosystem recognition benefit is theoretical | +| User ergonomics | `github.com/org/repo@v1` is more readable than `pkg:github/org/repo@v1` | + +**Door stays open:** The `#` fragment syntax is PURL-compatible. A future PURL adoption would extend the parser, not redesign it. + +## Work Breakdown + +### Core: Structured Dependencies with `select` (craft.yaml) + +**Current format** (string-only): +```yaml +dependencies: + acme: github.com/acme/skills@v1.0.0 +``` + +**New format** (string OR structured object): +```yaml +dependencies: + # Simple — install all skills (backward compatible) + tools: github.com/example/tools@v1.0.0 + + # Structured — install only selected skills + acme: + url: github.com/acme/skills@v1.0.0 + select: + - skills/docx + - skills/pdf +``` + +- `dependencies` values become `string | object` +- Object form has required `url` (DepURL string) and optional `select` (list of subpaths) +- When `select` is omitted or empty: install all exported skills (current behavior) +- When `select` is present: install only skills whose paths match the listed subpaths + +### Core: DepURL Subpath Extension + +Add a `Subpath` field to the `DepURL` struct for single-subpath references (e.g., in `craft get`): + +``` +github.com/acme/skills@v1.0.0#skills/docx +``` + +- Fragment separator: `#` (PURL-compatible) +- `ParseDepURL` extended to parse optional `#subpath` +- Single subpath per URL (for multi-select, use structured `select` in craft.yaml) + +### Core: Resolver Changes + +- `Manifest.Dependencies` type changes from `map[string]string` to `map[string]DependencySpec` +- `DependencySpec` is a type that can unmarshal from either a string or an object +- Resolver filters discovered skills against `select` list when present +- Multiple structured entries pointing to same package identity: fetch once, merge selections +- Pinfile records which subpaths were selected (for reproducibility) + +### Supporting: Interactive Preview in `craft add` + +When adding a dependency to a package with many skills: +``` +$ craft add acme github.com/acme/big-package@v1.0.0 +📦 acme has 12 skills: + [ ] docx [ ] pdf [ ] xlsx + [ ] pptx [ ] csv [ ] json + ... +Install all 12? [Y/n/select] +> select +Select skills (space to toggle, enter to confirm): + [x] docx + [x] pdf + [ ] xlsx + ... +``` + +- Fetch the package, discover skills, present interactive selection +- If user selects all: write simple string format +- If user selects subset: write structured format with `select` +- Skip interactivity with `--all` flag + +### Supporting: `craft get` Subpath Support + +``` +$ craft get github.com/acme/skills@v1.0.0#skills/docx +``` + +- Install a single skill from a multi-skill package +- Uses the DepURL `#subpath` extension +- Useful for quick, one-off installs without a manifest + +## Edge Cases + +| Scenario | Expected Handling | +|----------|-------------------| +| `select` path doesn't match any skill in package | Error: "skill path 'skills/foo' not found in acme" | +| `select` path matches a directory but it has no SKILL.md | Error: "no SKILL.md found at 'skills/foo'" | +| Dependency author removes a skill that consumer selects | Error at resolve time (pinfile integrity mismatch) | +| Multiple deps select different skills from same package | Merge selections, single fetch, install union | +| `select` with auto-discovered package (no craft.yaml) | Works — filter auto-discovered skills by path | +| Empty `select: []` | Treat as "all" (same as omitting `select`) | +| `craft get` with subpath on a package that has craft.yaml | Subpath overrides the package's own `skills` export list | + +## Architecture + +``` +craft.yaml (user) + │ + ├─ string dep ──────────────────► ParseDepURL ──► DepURL (no subpath) + │ │ + └─ structured dep ──► DependencySpec │ + ├─ url ──────────► ParseDepURL ──► DepURL │ + └─ select ───────► []string (subpaths) │ + ▼ + Resolver.Collect() + │ + ┌────────┴────────┐ + │ Has select? │ + │ Yes: filter │ + │ No: all skills │ + └────────┬────────┘ + ▼ + ResolvedDep (with SkillPaths) + │ + ▼ + Installer (unchanged) +``` + +## Codebase Fit + +| Area | Impact | +|------|--------| +| `internal/resolve/depurl.go` | Add `Subpath` field, extend `ParseDepURL` for `#fragment` | +| `internal/manifest/types.go` | New `DependencySpec` type, custom YAML unmarshaler | +| `internal/manifest/parse.go` | Handle string-or-object deserialization | +| `internal/manifest/validate.go` | Validate `select` paths, validate `url` field | +| `internal/manifest/write.go` | Serialize structured deps back to YAML | +| `internal/resolve/resolver.go` | Filter skills by `select` during discovery | +| `internal/resolve/discover.go` | Accept subpath filter parameter | +| `internal/pinfile/types.go` | Record selected paths in `ResolvedEntry` | +| `internal/cli/add.go` | Interactive skill preview/selection UX | +| `internal/cli/get.go` | Parse `#subpath` from URL argument | +| `internal/cli/install.go` | Pass select filters to resolver | +| Tests (all above) | New test cases for structured deps, subpath parsing, filtering | + +**Reuse opportunities:** +- `DepURL.Subpath` reuses existing `ParseDepURL` pipeline — minimal parser change +- Skill filtering can reuse existing `discoverSkillsForDep` with a path allowlist +- Interactive selection can reuse `internal/ui/` progress/tree formatting + +## Risk Assessment + +| Risk | Severity | Mitigation | +|------|----------|------------| +| Manifest format change breaks existing craft.yaml files | **High** | String format remains valid — this is purely additive | +| Structured deps complicate validation logic | Medium | Custom YAML unmarshaler isolates complexity | +| Subpath matching ambiguity (relative vs absolute, trailing slash) | Medium | Normalize paths, document exact matching rules | +| Interactive preview slows down non-interactive CI/CD usage | Low | `--all` flag bypasses interactivity; detect non-TTY | +| Future PURL adoption still requires significant work | Low | Accepted tradeoff — subpath syntax is compatible | + +## Critical Analysis + +**Value assessment:** High. Subset selection is the #1 missing feature for consuming large skill packages. The agentskills RFC has this as an explicit open question, and craft can lead by example. + +**Build vs. modify:** This is a modify — extending existing systems rather than replacing them. The DepURL struct gets one new field; the manifest format gains one new shape; the resolver adds one filter step. + +**Complexity budget:** The structured `DependencySpec` type is the biggest new concept. Custom YAML unmarshaling (string → simple, object → structured) is well-trodden Go pattern. Everything else is incremental. + +## Resolved Questions + +1. **Glob patterns in `select`?** → **No.** Explicit paths only. Predictability over convenience — avoids accidental skill creep from upstream changes. +2. **`exclude` counterpart?** → **No.** `select` only. One mechanism, no ambiguity. Users who want most skills list them explicitly. +3. **Pinfile key format?** → **Merged.** One package = one pinfile entry. Selections from multiple aliases are unioned. The pinfile records what was resolved; the manifest records who asked for what. +4. **`craft update` and new skills?** → **Inform only.** Show newly available skills with a hint (`Use 'craft add acme --select xlsx'`) but don't prompt or auto-install. + +## Session Notes + +- **Explored PURL adoption**: Decided against full adoption due to high integration cost vs. limited immediate benefit. Skills ecosystem is git-only today. +- **PURL compatibility preserved**: Using `#` fragment syntax ensures future PURL adoption path remains open. +- **Structured deps chosen over multi-entry**: User preferred `select` list in an object over multiple craft.yaml entries pointing to same package. Cleaner UX for multi-selection. +- **Interactive preview chosen**: User wants `craft add` to show available skills and let users select interactively, rather than silently installing everything. +- **No interface abstraction now**: DepURL stays a concrete struct. Refactor to interface only when/if PURL is actually adopted. +- **Originated from**: [agentskills/agentskills#210 comment by travbale](https://github.com/agentskills/agentskills/discussions/210#discussioncomment-16284365) diff --git a/.paw/work/feature-purl/WorkflowContext.md b/.paw/work/feature-purl/WorkflowContext.md new file mode 100644 index 0000000..837c6fd --- /dev/null +++ b/.paw/work/feature-purl/WorkflowContext.md @@ -0,0 +1,41 @@ +# WorkflowContext + +Work Title: Subpath Skill Selection +Work ID: feature-purl +Base Branch: main +Target Branch: feature/purl +Execution Mode: current-checkout +Repository Identity: github.com/erdemtuna/craft@88c51759a4146aae008a4201853f64857aad3f97 +Execution Binding: none +Workflow Mode: full +Review Strategy: local +Review Policy: planning-only +Session Policy: continuous +Final Agent Review: enabled +Final Review Mode: multi-model +Final Review Interactive: smart +Final Review Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 +Final Review Specialists: all +Final Review Interaction Mode: parallel +Final Review Specialist Models: none +Final Review Perspectives: auto +Final Review Perspective Cap: 2 +Implementation Model: none +Plan Generation Mode: single-model +Plan Generation Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 +Planning Docs Review: enabled +Planning Review Mode: multi-model +Planning Review Interactive: smart +Planning Review Models: gpt-5.2, gemini-3-pro-preview, claude-opus-4.6 +Planning Review Specialists: all +Planning Review Interaction Mode: parallel +Planning Review Specialist Models: none +Planning Review Perspectives: auto +Planning Review Perspective Cap: 2 +Custom Workflow Instructions: none +Initial Prompt: Add subpath-based skill selection to craft using PURL-compatible #fragment syntax, with structured dependency format in craft.yaml and interactive preview in craft add. See WorkShaping.md for full design decisions. +Issue URL: https://github.com/agentskills/agentskills/discussions/210#discussioncomment-16284365 +Remote: origin +Artifact Lifecycle: commit-and-persist +Artifact Paths: auto-derived +Additional Inputs: none diff --git a/README.md b/README.md index 97d2dd5..dedd2e1 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,10 @@ Found a skill package you want to use? One command: ```bash $ craft get github.com/acme/company-standards@v2.1.0 Installed 2 skill(s) from 1 package(s) to /home/user/.claude/skills + +# Install a specific skill from a large package +$ craft get github.com/acme/skills@v1.0.0#skills/docx +Installed 1 skill(s) from 1 package(s) to /home/user/.claude/skills ``` That's it — skills are installed to your AI agent and tracked for updates. No project setup needed. @@ -88,6 +92,13 @@ skills: dependencies: standards: github.com/acme/company-standards@v2.1.0 + + # Or install only specific skills from a large package + acme: + url: github.com/acme/skills@v1.0.0 + select: + - skills/docx + - skills/pdf ``` Meanwhile, the docs team's `doc-generator` skill and the infra team's `api-designer` skill all depend on the same `company-standards` package. When the platform team updates the conventions to v2.2.0, every team bumps one version number and gets the update — no copy-paste drift, no stale rules. @@ -175,9 +186,9 @@ Operate on the project manifest (`craft.yaml`) in the current directory. | Command | Description | |---------|-------------| | `craft init` | Interactive package setup with skill auto-discovery | -| `craft add [alias] ` | Add a dependency to `craft.yaml` | +| `craft add [alias] [--all]` | Add a dependency to `craft.yaml` (interactive skill selection for multi-skill packages) | | `craft install` | Resolve, pin, and vendor dependencies to `forge/` | -| `craft update [alias]` | Update project dependencies (re-resolve branches, skip commit pins, bump tags) | +| `craft update [alias]` | Update project dependencies (re-resolve branches, skip commit pins, bump tags); informs about newly available skills in selectively installed packages | | `craft remove ` | Remove a dependency and clean up orphaned skills from `forge/` | | `craft list` | List project dependencies with versions and skill counts | | `craft tree` | Print project dependency tree | @@ -202,6 +213,10 @@ Install skills from any repository directly into your AI agent's skill directory $ craft get github.com/acme/company-standards@v2.1.0 Installed 2 skill(s) from 1 package(s) to /home/user/.claude/skills +# Install a specific skill from a package using #subpath +$ craft get github.com/acme/skills@v1.0.0#skills/docx +Installed 1 skill(s) from 1 package(s) to /home/user/.claude/skills + # Install with a custom alias $ craft get standards github.com/acme/company-standards@v2.1.0 @@ -218,6 +233,8 @@ Global state is tracked at `~/.craft/craft.yaml` and `~/.craft/craft.pin.yaml` Add a dependency to your `craft.yaml`. The dependency is resolved and verified before the manifest is updated. +For packages with multiple skills, `craft add` presents an interactive prompt so you can choose which skills to include. Use `--all` to skip the prompt and install everything. In non-TTY environments (e.g., CI/CD), the prompt is automatically skipped. + ```bash # Add with auto-derived alias (uses repo name) $ craft add github.com/acme/utility-skills@v1.0.0 @@ -228,6 +245,9 @@ Added "utility-skills" → github.com/acme/utility-skills@v1.0.0 # Add with custom alias $ craft add utils github.com/acme/utility-skills@v1.0.0 +# Skip interactive selection — install all skills +$ craft add --all github.com/acme/productivity-suite@v2.0.0 + # Add and immediately vendor to forge/ $ craft add --install github.com/acme/utility-skills@v1.0.0 @@ -327,6 +347,7 @@ $ echo $? | Flag | Available On | Description | |------|-------------|-------------| | `--dry-run` | `install`, `update`, `get` | Show what would happen without making any changes | +| `--all` | `add` | Skip interactive skill selection and install all skills from the package | | `--target ` | `get`, `install -g`, `update -g`, `remove` | Override agent auto-detection with a custom install path. Rejected on project-scope `install` and `update` (skills vendor to `forge/`). | ## Manifest Reference (`craft.yaml`) @@ -340,10 +361,17 @@ license: MIT # Optional skills: # Relative paths to skill directories - ./skills/my-skill -dependencies: # alias → host/org/repo@ - utils: github.com/example/util-skills@v1.0.0 # tagged version +dependencies: # alias → URL string or structured object + utils: github.com/example/util-skills@v1.0.0 # tagged version (all skills) tools: github.com/example/dev-tools@branch:main # branch tracking legacy: github.com/example/old-skills@abc1234def5678 # commit pin + + # Structured format — install only selected skills + acme: + url: github.com/example/big-suite@v2.0.0 + select: + - skills/docx + - skills/pdf ``` ## `SKILL.md` @@ -380,7 +408,7 @@ The flat directory name is derived from the composite key: slashes become `--`, ## Known Limitations - **go-git SSH limitations** — craft uses [go-git](https://github.com/go-git/go-git) for git operations. This means no `~/.ssh/config` ProxyJump support, no hardware token (YubiKey) auth, and no agent forwarding. Set `GITHUB_TOKEN` or `CRAFT_TOKEN` for private repos as a reliable alternative. -- **No monorepo subpath support** — dependency URLs point to whole repositories (`github.com/org/repo@v1.0.0`). Subpath support (e.g., `repo/path/to/skills@v1`) is designed for but not yet implemented. +- **No glob patterns in skill selection** — `select` paths must be explicit (e.g., `skills/docx`); glob patterns like `skills/doc*` are not supported. - **No pre-release versions** — tagged version refs must be strict semver (`vMAJOR.MINOR.PATCH`). Pre-release suffixes like `-beta.1` are rejected. (Commit SHA and branch refs are unaffected.) - **No version ranges** — tagged dependencies use exact versions. `craft update` bumps tags to the latest available, re-resolves branch deps to HEAD, and skips commit pins; there are no `^` or `~` range constraints. - **Cache grows unbounded** — use `craft cache clean` periodically to reclaim disk space. diff --git a/internal/cli/add.go b/internal/cli/add.go index 7f3ef0f..4d06d19 100644 --- a/internal/cli/add.go +++ b/internal/cli/add.go @@ -7,11 +7,14 @@ import ( "path/filepath" "strings" + "github.com/erdemtuna/craft/internal/fetch" installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/manifest" "github.com/erdemtuna/craft/internal/pinfile" "github.com/erdemtuna/craft/internal/resolve" + "github.com/erdemtuna/craft/internal/ui" "github.com/spf13/cobra" + "golang.org/x/term" ) var addInstall bool @@ -33,6 +36,7 @@ The dependency is verified by resolving it before updating the manifest.`, func init() { addCmd.Flags().BoolVar(&addInstall, "install", false, "Run install after adding the dependency (vendors to forge/)") + addCmd.Flags().Bool("all", false, "Install all skills without interactive selection") } func runAdd(cmd *cobra.Command, args []string) error { @@ -50,6 +54,9 @@ func runAdd(cmd *cobra.Command, args []string) error { return fmt.Errorf("%w\n hint: expected format: host/org/repo@v1.0.0, host/org/repo@, or host/org/repo@branch:", err) } + // Strip #fragment from URL — subpath is handled via Select + depURL = parsed.PackageIdentity() + "@" + parsed.RefString() + // Warn about non-tagged dependencies if parsed.RefType != resolve.RefTypeTag { cmd.PrintErrln("⚠ Non-tagged dependency: " + depURL) @@ -81,28 +88,47 @@ func runAdd(cmd *cobra.Command, args []string) error { } // Check for existing dependency + addAll, _ := cmd.Flags().GetBool("all") + isInteractive := term.IsTerminal(int(os.Stdin.Fd())) && !addAll isUpdate := false if existing, ok := m.Dependencies[alias]; ok { isUpdate = true - if existing == depURL { + if existing.URL == depURL && !isInteractive { cmd.Printf("Dependency %q is already at %s — nothing to do.\n", alias, depURL) return nil } - cmd.Printf("Updating %q: %s → %s\n", alias, existing, depURL) + if existing.URL != depURL { + cmd.Printf("Updating %q: %s → %s\n", alias, existing.URL, depURL) + } } // Add dependency in memory if m.Dependencies == nil { - m.Dependencies = make(map[string]string) + m.Dependencies = make(map[string]manifest.DependencySpec) + } + if parsed.Subpath != "" { + m.Dependencies[alias] = manifest.DependencySpec{URL: depURL, Select: []string{parsed.Subpath}} + } else { + m.Dependencies[alias] = manifest.DependencySpec{URL: depURL} } - m.Dependencies[alias] = depURL - // Validate by resolving with full manifest fetcher, err := newFetcher() if err != nil { return err } + // Interactive skill selection: if running in a TTY with multiple + // skills available, let the user pick which skills to include. + if isInteractive && parsed.Subpath == "" { + selected, err := discoverAndSelect(cmd, parsed, fetcher) + if err != nil { + cmd.PrintErrf("⚠ Could not discover skills: %v — installing all.\n", err) + } else if len(selected) > 0 { + m.Dependencies[alias] = manifest.DependencySpec{URL: depURL, Select: selected} + } + } + + // Validate by resolving with full manifest // Load existing pinfile to preserve pins for unchanged deps var existingPinfile *pinfile.Pinfile pfPath := filepath.Join(root, "craft.pin.yaml") @@ -185,3 +211,74 @@ func runAdd(cmd *cobra.Command, args []string) error { return nil } + +// discoverAndSelect fetches the skill list for a package and prompts the +// user to pick a subset when multiple skills are found. Returns nil (meaning +// "all") when the user selects everything or when only one skill exists. +// Returns the selected skill directory paths when a subset is chosen. +func discoverAndSelect(cmd *cobra.Command, parsed *resolve.DepURL, fetcher fetch.GitFetcher) ([]string, error) { + cloneURL := fetch.NormalizeCloneURL(parsed.PackageIdentity()) + + // Resolve the ref to a commit SHA + var ( + commitSHA string + err error + ) + if parsed.RefType == resolve.RefTypeCommit { + commitSHA = parsed.Ref + } else { + commitSHA, err = fetcher.ResolveRef(cloneURL, parsed.GitRef()) + if err != nil { + return nil, fmt.Errorf("resolving ref: %w", err) + } + } + + // List all files and discover skills + paths, err := fetcher.ListTree(cloneURL, commitSHA) + if err != nil { + return nil, fmt.Errorf("listing tree: %w", err) + } + + skills, err := resolve.DiscoverSkills(paths, func(path string) ([]byte, error) { + files, err := fetcher.ReadFiles(cloneURL, commitSHA, []string{path}) + if err != nil { + return nil, err + } + content, ok := files[path] + if !ok { + return nil, fmt.Errorf("file not found: %s", path) + } + return content, nil + }) + if err != nil { + return nil, fmt.Errorf("discovering skills: %w", err) + } + + if len(skills) <= 1 { + return nil, nil + } + + // Build display names + names := make([]string, len(skills)) + for i, s := range skills { + names[i] = s.Name + } + + cmd.Printf("Found %d skills in package:\n", len(skills)) + indices, err := ui.MultiSelect("Select skills to include:", names, cmd.OutOrStderr(), os.Stdin) + if err != nil { + return nil, err + } + + // nil means "all" + if indices == nil { + return nil, nil + } + + // Map selected indices to skill directory paths + selected := make([]string, len(indices)) + for i, idx := range indices { + selected[i] = skills[idx].Dir + } + return selected, nil +} diff --git a/internal/cli/get.go b/internal/cli/get.go index 76fea13..c13277e 100644 --- a/internal/cli/get.go +++ b/internal/cli/get.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "slices" "strings" installlib "github.com/erdemtuna/craft/internal/install" @@ -72,7 +73,8 @@ func runGet(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("%w\n hint: expected format: host/org/repo@v1.0.0, host/org/repo@, or host/org/repo@branch:", err) } - deps = append(deps, depEntry{alias: args[0], url: args[1], parsed: parsed, explicitAlias: true}) + cleanURL := parsed.PackageIdentity() + "@" + parsed.RefString() + deps = append(deps, depEntry{alias: args[0], url: cleanURL, parsed: parsed, explicitAlias: true}) } else { // All args are URLs for _, arg := range args { @@ -80,7 +82,8 @@ func runGet(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("%w\n hint: expected format: host/org/repo@v1.0.0, host/org/repo@, or host/org/repo@branch:", err) } - deps = append(deps, depEntry{alias: parsed.Repo, url: arg, parsed: parsed}) + cleanURL := parsed.PackageIdentity() + "@" + parsed.RefString() + deps = append(deps, depEntry{alias: parsed.Repo, url: cleanURL, parsed: parsed}) } } @@ -106,22 +109,23 @@ func runGet(cmd *cobra.Command, args []string) error { m = &manifest.Manifest{ SchemaVersion: 1, Name: "global", - Dependencies: make(map[string]string), + Dependencies: make(map[string]manifest.DependencySpec), } } if m.Dependencies == nil { - m.Dependencies = make(map[string]string) + m.Dependencies = make(map[string]manifest.DependencySpec) } // Check for already-installed deps and prompt if different isTTY := term.IsTerminal(int(os.Stdin.Fd())) for i, dep := range deps { - existing, ok := m.Dependencies[dep.alias] + existingSpec, ok := m.Dependencies[dep.alias] if !ok { continue } - if existing == dep.url { + newSelect := selectFromSubpath(dep.parsed.Subpath) + if existingSpec.URL == dep.url && slices.Equal(existingSpec.Select, newSelect) { cmd.Printf("%q is already installed at %s — skipping.\n", dep.alias, dep.url) // Mark as skip by clearing URL deps[i].url = "" @@ -130,9 +134,9 @@ func runGet(cmd *cobra.Command, args []string) error { // Determine if this is the same package (version change) or a // completely different package that happens to share the repo name. - existingParsed, existingErr := resolve.ParseDepURL(existing) + existingParsed, existingErr := resolve.ParseDepURL(existingSpec.URL) if existingErr != nil { - return fmt.Errorf("existing dependency %q has invalid URL %s: %w", dep.alias, existing, existingErr) + return fmt.Errorf("existing dependency %q has invalid URL %s: %w", dep.alias, existingSpec.URL, existingErr) } samePackage := existingParsed.PackageIdentity() == dep.parsed.PackageIdentity() @@ -140,16 +144,16 @@ func runGet(cmd *cobra.Command, args []string) error { if dep.explicitAlias { // User explicitly chose this alias — don't silently rename it return fmt.Errorf("alias %q is already used by a different package (%s)\n hint: choose a different alias: craft get %s", - dep.alias, existing, dep.url) + dep.alias, existingSpec.URL, dep.url) } // Auto-derived alias — derive a unique one using org-repo // so both packages can coexist without prompting. newAlias := dep.parsed.Org + "-" + dep.parsed.Repo if _, collision := m.Dependencies[newAlias]; collision { return fmt.Errorf("alias %q conflicts with a different package (%s)\n hint: use 'craft get %s' to provide a distinct alias", - dep.alias, existing, dep.url) + dep.alias, existingSpec.URL, dep.url) } - cmd.Printf("Alias %q is used by %s — using %q instead.\n", dep.alias, existing, newAlias) + cmd.Printf("Alias %q is used by %s — using %q instead.\n", dep.alias, existingSpec.URL, newAlias) deps[i].alias = newAlias continue } @@ -157,9 +161,9 @@ func runGet(cmd *cobra.Command, args []string) error { // Same package, different version — prompt to update if !isTTY { return fmt.Errorf("%q is already installed at %s (requested %s)\n hint: use an interactive terminal to confirm updates, or use craft update -g", - dep.alias, existing, dep.url) + dep.alias, existingSpec.URL, dep.url) } - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "%q is currently at %s. Update to %s? [y/N]: ", dep.alias, existing, dep.url) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "%q is currently at %s. Update to %s? [y/N]: ", dep.alias, existingSpec.URL, dep.url) scanner := bufio.NewScanner(os.Stdin) if !scanner.Scan() || !strings.HasPrefix(strings.ToLower(strings.TrimSpace(scanner.Text())), "y") { cmd.Printf("Skipping %q.\n", dep.alias) @@ -193,7 +197,11 @@ func runGet(cmd *cobra.Command, args []string) error { // Add all deps to manifest for _, dep := range activeDeps { - m.Dependencies[dep.alias] = dep.url + spec := manifest.DependencySpec{ + URL: dep.url, + Select: selectFromSubpath(dep.parsed.Subpath), + } + m.Dependencies[dep.alias] = spec // Warn about non-tagged dependencies if dep.parsed.RefType != resolve.RefTypeTag { cmd.PrintErrln("⚠ Non-tagged dependency: " + dep.url) @@ -305,3 +313,12 @@ func runGet(cmd *cobra.Command, args []string) error { return nil } + +// selectFromSubpath returns a single-element Select slice when subpath is +// non-empty, or nil when there is no subpath (meaning "install all skills"). +func selectFromSubpath(subpath string) []string { + if subpath == "" { + return nil + } + return []string{subpath} +} diff --git a/internal/cli/global_test.go b/internal/cli/global_test.go index 2c487fb..422725d 100644 --- a/internal/cli/global_test.go +++ b/internal/cli/global_test.go @@ -61,6 +61,7 @@ func TestGlobalFlag(t *testing.T) { f := rootCmd.PersistentFlags().Lookup("global") if f == nil { t.Fatal("expected --global flag to be registered on rootCmd") + return } if f.Shorthand != "g" { t.Errorf("expected shorthand 'g', got %q", f.Shorthand) diff --git a/internal/cli/list.go b/internal/cli/list.go index 6e780ff..cc277b2 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -41,8 +41,8 @@ func runList(cmd *cobra.Command, args []string) error { // Build alias-to-URL lookup from manifest urlToAlias := make(map[string]string) - for alias, depURL := range m.Dependencies { - parsed, err := resolve.ParseDepURL(depURL) + for alias, depSpec := range m.Dependencies { + parsed, err := resolve.ParseDepURL(depSpec.URL) if err != nil { continue } diff --git a/internal/cli/outdated.go b/internal/cli/outdated.go index 24826e4..e3c3a08 100644 --- a/internal/cli/outdated.go +++ b/internal/cli/outdated.go @@ -68,7 +68,8 @@ func runOutdated(cmd *cobra.Command, args []string) error { sort.Strings(aliases) for _, alias := range aliases { - depURL := m.Dependencies[alias] + depSpec := m.Dependencies[alias] + depURL := depSpec.URL parsed, err := resolve.ParseDepURL(depURL) if err != nil { results = append(results, depStatus{alias: alias, err: fmt.Errorf("invalid URL: %w", err)}) diff --git a/internal/cli/remove.go b/internal/cli/remove.go index 63ae7e6..3632406 100644 --- a/internal/cli/remove.go +++ b/internal/cli/remove.go @@ -66,7 +66,7 @@ func runRemove(cmd *cobra.Command, args []string) error { } // Check alias exists - depURL, ok := m.Dependencies[alias] + depSpec, ok := m.Dependencies[alias] if !ok { available := availableAliases(m.Dependencies) return fmt.Errorf("dependency %q not found in craft.yaml\n hint: available aliases: %s", alias, available) @@ -75,6 +75,7 @@ func runRemove(cmd *cobra.Command, args []string) error { // Identify skills from the removed dependency (from pinfile) pf, pfErr := pinfile.ParseFile(pfPath) + depURL := depSpec.URL var removedSkills []string if pfErr == nil { // Find the pinfile entry for this dependency @@ -204,7 +205,7 @@ func cleanEmptyParents(root, dir string) { } // availableAliases formats the available dependency aliases for error messages. -func availableAliases(deps map[string]string) string { +func availableAliases(deps map[string]manifest.DependencySpec) string { if len(deps) == 0 { return "(none)" } diff --git a/internal/cli/remove_test.go b/internal/cli/remove_test.go index 6bd1d6d..48e9d72 100644 --- a/internal/cli/remove_test.go +++ b/internal/cli/remove_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/erdemtuna/craft/internal/manifest" ) func TestRunRemove_ExistingDep(t *testing.T) { @@ -243,9 +245,9 @@ resolved: } func TestAvailableAliases(t *testing.T) { - deps := map[string]string{ - "zebra": "z", - "alpha": "a", + deps := map[string]manifest.DependencySpec{ + "zebra": {URL: "z"}, + "alpha": {URL: "a"}, } got := availableAliases(deps) if got != "alpha, zebra" { diff --git a/internal/cli/tree.go b/internal/cli/tree.go index a987428..1ead9e2 100644 --- a/internal/cli/tree.go +++ b/internal/cli/tree.go @@ -36,8 +36,8 @@ func runTree(cmd *cobra.Command, args []string) error { // Build alias lookup from manifest urlToAlias := make(map[string]string) - for alias, depURL := range m.Dependencies { - parsed, err := resolve.ParseDepURL(depURL) + for alias, depSpec := range m.Dependencies { + parsed, err := resolve.ParseDepURL(depSpec.URL) if err != nil { continue } diff --git a/internal/cli/update.go b/internal/cli/update.go index 1bc934d..eecff38 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -103,11 +103,12 @@ func runUpdate(cmd *cobra.Command, args []string) error { // Find latest versions for targeted deps progress.Start("Checking for updates...") updated := false - for alias, depURL := range m.Dependencies { + for alias, depSpec := range m.Dependencies { if targetAlias != "" && alias != targetAlias { continue } + depURL := depSpec.URL parsed, err := resolve.ParseDepURL(depURL) if err != nil { return fmt.Errorf("invalid dependency URL for %q: %w", alias, err) @@ -158,7 +159,7 @@ func runUpdate(cmd *cobra.Command, args []string) error { if semver.Compare(strings.TrimPrefix(latest, "v"), parsed.Version) > 0 { newURL := parsed.WithVersion(latest) - m.Dependencies[alias] = newURL + m.Dependencies[alias] = manifest.DependencySpec{URL: newURL, Select: depSpec.Select} updated = true cmd.Printf(" %s: %s → %s\n", alias, parsed.GitRef(), latest) } @@ -177,9 +178,9 @@ func runUpdate(cmd *cobra.Command, args []string) error { // Resolve before writing anything to disk progress.Update("Resolving updated dependencies...") forceResolve := make(map[string]bool) - for alias, depURL := range m.Dependencies { + for alias, depSpec := range m.Dependencies { if targetAlias == "" || alias == targetAlias { - forceResolve[depURL] = true + forceResolve[depSpec.URL] = true } } @@ -193,6 +194,37 @@ func runUpdate(cmd *cobra.Command, args []string) error { return fmt.Errorf("resolution failed: %w", err) } + // Notify about newly discovered skills in selectively-installed deps. + for _, dep := range result.Resolved { + if len(dep.Select) == 0 { + continue + } + installed := make(map[string]bool, len(dep.SkillPaths)) + for _, p := range dep.SkillPaths { + installed[p] = true + } + var newSkills []string + for _, p := range dep.AllSkillPaths { + if !installed[p] { + newSkills = append(newSkills, p) + } + } + if len(newSkills) > 0 { + alias := dep.Alias + if alias == "" { + for a, ds := range m.Dependencies { + if ds.URL == dep.URL { + alias = a + break + } + } + } + if alias != "" { + cmd.Printf("ℹ New skills available in %s: %s\n Use 'craft add' to include them.\n", alias, strings.Join(newSkills, ", ")) + } + } + } + // Dry-run: show what would change and exit if updateDryRun { progress.Done("Dry-run complete") diff --git a/internal/cli/update_test.go b/internal/cli/update_test.go index 442793a..de1e432 100644 --- a/internal/cli/update_test.go +++ b/internal/cli/update_test.go @@ -3,10 +3,12 @@ package cli import ( "os" "path/filepath" + "reflect" "strings" "testing" "github.com/erdemtuna/craft/internal/manifest" + "github.com/erdemtuna/craft/internal/resolve" "github.com/erdemtuna/craft/internal/semver" ) @@ -103,8 +105,8 @@ func TestWriteManifestAtomic(t *testing.T) { SchemaVersion: 1, Name: "test-pkg", Skills: []string{"skills/lint"}, - Dependencies: map[string]string{ - "tools": "github.com/org/tools@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/org/tools@v1.0.0"}, }, } @@ -171,3 +173,27 @@ func TestWriteManifestAtomic_BadPath(t *testing.T) { t.Error("expected error for nonexistent directory") } } + +func TestUpdatePreservesSelect(t *testing.T) { + current := manifest.DependencySpec{ + URL: "github.com/acme/skills@v1.0.0", + Select: []string{"skills/docx", "skills/pdf"}, + } + + parsed, err := resolve.ParseDepURL(current.URL) + if err != nil { + t.Fatalf("ParseDepURL() error = %v", err) + } + + updated := manifest.DependencySpec{ + URL: parsed.WithVersion("v1.2.0"), + Select: current.Select, + } + + if updated.URL != "github.com/acme/skills@v1.2.0" { + t.Errorf("updated URL = %q, want %q", updated.URL, "github.com/acme/skills@v1.2.0") + } + if !reflect.DeepEqual(updated.Select, current.Select) { + t.Errorf("updated Select = %v, want %v", updated.Select, current.Select) + } +} diff --git a/internal/manifest/parse_test.go b/internal/manifest/parse_test.go index f22cb07..1121df7 100644 --- a/internal/manifest/parse_test.go +++ b/internal/manifest/parse_test.go @@ -44,8 +44,8 @@ metadata: if len(m.Dependencies) != 1 { t.Fatalf("Dependencies length = %d, want 1", len(m.Dependencies)) } - if m.Dependencies["git-ops"] != "github.com/example/git@v1.0.0" { - t.Errorf("Dependencies[git-ops] = %q", m.Dependencies["git-ops"]) + if m.Dependencies["git-ops"].URL != "github.com/example/git@v1.0.0" { + t.Errorf("Dependencies[git-ops].URL = %q", m.Dependencies["git-ops"].URL) } if m.Metadata["author"] != "tester" { t.Errorf("Metadata[author] = %q", m.Metadata["author"]) diff --git a/internal/manifest/types.go b/internal/manifest/types.go index d0596d5..d829af4 100644 --- a/internal/manifest/types.go +++ b/internal/manifest/types.go @@ -2,6 +2,49 @@ // validation, and serialization. package manifest +import ( + "fmt" + "strings" + + "gopkg.in/yaml.v3" +) + +// DependencySpec represents a dependency in craft.yaml. +// It can be either a simple URL string or a structured object with url and select fields. +type DependencySpec struct { + // URL is the dependency URL (host/org/repo@ref). + URL string + // Select is an optional list of skill subpaths to install from this dependency. + // When empty, all exported skills are installed. + Select []string +} + +// UnmarshalYAML implements custom YAML unmarshaling to handle both string +// and object forms of dependency declarations. +func (d *DependencySpec) UnmarshalYAML(value *yaml.Node) error { + if value.Kind == yaml.ScalarNode { + d.URL = value.Value + return nil + } + if value.Kind == yaml.MappingNode { + type raw struct { + URL string `yaml:"url"` + Select []string `yaml:"select"` + } + var r raw + if err := value.Decode(&r); err != nil { + return err + } + if r.URL == "" { + return fmt.Errorf("structured dependency requires 'url' field") + } + d.URL = r.URL + d.Select = normalizeSelectPaths(r.Select) + return nil + } + return fmt.Errorf("dependency must be a string or object, got %v", value.Kind) +} + // Manifest represents a craft.yaml package manifest. type Manifest struct { // SchemaVersion is the manifest schema version (always 1 for this release). @@ -19,9 +62,25 @@ type Manifest struct { // Skills is the list of relative paths to skill directories. Skills []string `yaml:"skills"` - // Dependencies maps aliases to dependency URLs (host/org/repo@vMAJOR.MINOR.PATCH). - Dependencies map[string]string `yaml:"dependencies,omitempty"` + // Dependencies maps aliases to dependency specifications. + Dependencies map[string]DependencySpec `yaml:"dependencies,omitempty"` // Metadata holds arbitrary key-value pairs for extensibility. Metadata map[string]string `yaml:"metadata,omitempty"` } + +// normalizeSelectPaths trims whitespace, strips leading "./", and strips +// trailing "/" from each select path. +func normalizeSelectPaths(paths []string) []string { + if len(paths) == 0 { + return paths + } + out := make([]string, len(paths)) + for i, p := range paths { + p = strings.TrimSpace(p) + p = strings.TrimPrefix(p, "./") + p = strings.TrimSuffix(p, "/") + out[i] = p + } + return out +} diff --git a/internal/manifest/validate.go b/internal/manifest/validate.go index 0b5f208..7a4915b 100644 --- a/internal/manifest/validate.go +++ b/internal/manifest/validate.go @@ -3,6 +3,7 @@ package manifest import ( "fmt" "regexp" + "strings" ) // namePattern matches valid package names: lowercase letter followed by @@ -51,10 +52,19 @@ func validateCommon(m *Manifest) []error { errs = append(errs, fmt.Errorf("name: %q does not match required format (lowercase alphanumeric with hyphens, e.g. 'my-package')", m.Name)) } - // validate dependency URL format for each entry - for alias, url := range m.Dependencies { - if !depURLPattern.MatchString(url) { - errs = append(errs, fmt.Errorf("dependencies[%q]: %q does not match required format (host/org/repo@ where ref is vX.Y.Z, commit SHA, or branch:)", alias, url)) + // validate dependency URL format and select paths for each entry + for alias, dep := range m.Dependencies { + if !depURLPattern.MatchString(dep.URL) { + errs = append(errs, fmt.Errorf("dependencies[%q]: %q does not match required format (host/org/repo@ where ref is vX.Y.Z, commit SHA, or branch:)", alias, dep.URL)) + } + for i, sel := range dep.Select { + if sel == "" { + errs = append(errs, fmt.Errorf("dependencies[%q].select[%d]: path must not be empty", alias, i)) + } else if strings.HasPrefix(sel, "/") { + errs = append(errs, fmt.Errorf("dependencies[%q].select: %q must be a relative path (no leading '/')", alias, sel)) + } else if strings.Contains(sel, "..") { + errs = append(errs, fmt.Errorf("dependencies[%q].select: %q must not contain '..' (path traversal)", alias, sel)) + } } } diff --git a/internal/manifest/validate_test.go b/internal/manifest/validate_test.go index 4d0b1d9..6730aad 100644 --- a/internal/manifest/validate_test.go +++ b/internal/manifest/validate_test.go @@ -22,8 +22,8 @@ func TestValidateWithDependencies(t *testing.T) { SchemaVersion: 1, Name: "my-package", Skills: []string{"./skills/one"}, - Dependencies: map[string]string{ - "git-ops": "github.com/example/git@v1.0.0", + Dependencies: map[string]DependencySpec{ + "git-ops": {URL: "github.com/example/git@v1.0.0"}, }, } errs := Validate(m) @@ -130,7 +130,7 @@ func TestValidateDependencyURLFormats(t *testing.T) { SchemaVersion: 1, Name: "test", Skills: []string{"./skill"}, - Dependencies: map[string]string{"dep": tc.url}, + Dependencies: map[string]DependencySpec{"dep": {URL: tc.url}}, } errs := Validate(m) hasDepErr := false @@ -204,7 +204,7 @@ func TestValidateGlobal_InvalidDepURL(t *testing.T) { SchemaVersion: 1, Name: "test", Skills: []string{}, - Dependencies: map[string]string{"bad": "not-a-valid-url"}, + Dependencies: map[string]DependencySpec{"bad": {URL: "not-a-valid-url"}}, } errs := ValidateGlobal(m) if len(errs) != 1 { @@ -218,7 +218,7 @@ func TestValidateGlobal_MultipleErrors(t *testing.T) { SchemaVersion: 2, Name: "", Skills: []string{}, - Dependencies: map[string]string{"bad": "not-a-valid-url"}, + Dependencies: map[string]DependencySpec{"bad": {URL: "not-a-valid-url"}}, } errs := ValidateGlobal(m) if len(errs) < 2 { @@ -231,7 +231,7 @@ func TestValidateGlobal_ValidWithDeps(t *testing.T) { SchemaVersion: 1, Name: "global", Skills: []string{}, - Dependencies: map[string]string{"dep": "github.com/org/repo@v1.0.0"}, + Dependencies: map[string]DependencySpec{"dep": {URL: "github.com/org/repo@v1.0.0"}}, } errs := ValidateGlobal(m) if len(errs) != 0 { @@ -245,3 +245,86 @@ func assertContains(t *testing.T, s, substr string) { t.Errorf("Expected %q to contain %q", s, substr) } } + +func TestValidateSelectPaths(t *testing.T) { + tests := []struct { + name string + selectVal []string + wantErr string + }{ + { + name: "valid select paths", + selectVal: []string{"skills/docx", "skills/pdf"}, + }, + { + name: "absolute path rejected", + selectVal: []string{"/skills/docx"}, + wantErr: "must be a relative path", + }, + { + name: "path traversal rejected", + selectVal: []string{"skills/../secret"}, + wantErr: "must not contain '..'", + }, + { + name: "empty string rejected", + selectVal: []string{""}, + wantErr: "must not be empty", + }, + { + name: "nested path valid", + selectVal: []string{"skills/nested/deep/docx"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := &Manifest{ + SchemaVersion: 1, + Name: "test-pkg", + Skills: []string{"./skills/one"}, + Dependencies: map[string]DependencySpec{ + "acme": { + URL: "github.com/acme/skills@v1.0.0", + Select: tt.selectVal, + }, + }, + } + errs := Validate(m) + if tt.wantErr == "" { + if len(errs) != 0 { + t.Errorf("Expected no errors, got %v", errs) + } + } else { + found := false + for _, err := range errs { + if strings.Contains(err.Error(), tt.wantErr) { + found = true + break + } + } + if !found { + t.Errorf("Expected error containing %q, got %v", tt.wantErr, errs) + } + } + }) + } +} + +func TestValidateStructuredDepMissingURL(t *testing.T) { + yamlInput := ` +schema_version: 1 +name: test-pkg +skills: + - ./skills/one +dependencies: + acme: + select: + - skills/docx +` + m, err := Parse(strings.NewReader(yamlInput)) + if err == nil { + t.Fatalf("Expected parse error for structured dep without url, got manifest: %+v", m) + } + assertContains(t, err.Error(), "url") +} diff --git a/internal/manifest/write.go b/internal/manifest/write.go index 0a09ffb..aad9ba0 100644 --- a/internal/manifest/write.go +++ b/internal/manifest/write.go @@ -29,7 +29,7 @@ func Write(m *Manifest, w io.Writer) error { addStringSlice(mapping, "skills", m.Skills) if len(m.Dependencies) > 0 { - addStringMap(mapping, "dependencies", m.Dependencies) + addDependencies(mapping, "dependencies", m.Dependencies) } if len(m.Metadata) > 0 { addStringMap(mapping, "metadata", m.Metadata) @@ -101,3 +101,51 @@ func addStringMap(mapping *yaml.Node, key string, m map[string]string) { mapping.Content = append(mapping.Content, keyNode, mapNode) } + +// addDependencies adds a map[string]DependencySpec as a mapping node with sorted keys. +// Simple dependencies (no Select) are written as scalar values; structured +// dependencies (with Select) are written as mapping nodes with url and select fields. +func addDependencies(mapping *yaml.Node, key string, deps map[string]DependencySpec) { + keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key} + mapNode := &yaml.Node{Kind: yaml.MappingNode} + + keys := make([]string, 0, len(deps)) + for k := range deps { + keys = append(keys, k) + } + sort.Strings(keys) + + for _, alias := range keys { + aliasNode := &yaml.Node{Kind: yaml.ScalarNode, Value: alias} + dep := deps[alias] + + if len(dep.Select) == 0 { + // Write as simple scalar value. + mapNode.Content = append(mapNode.Content, aliasNode, &yaml.Node{ + Kind: yaml.ScalarNode, + Value: dep.URL, + }) + } else { + // Write as structured object with url and select fields. + objNode := &yaml.Node{Kind: yaml.MappingNode} + + objNode.Content = append(objNode.Content, + &yaml.Node{Kind: yaml.ScalarNode, Value: "url"}, + &yaml.Node{Kind: yaml.ScalarNode, Value: dep.URL}, + ) + + selKey := &yaml.Node{Kind: yaml.ScalarNode, Value: "select"} + selSeq := &yaml.Node{Kind: yaml.SequenceNode} + for _, s := range dep.Select { + selSeq.Content = append(selSeq.Content, + &yaml.Node{Kind: yaml.ScalarNode, Value: s}, + ) + } + objNode.Content = append(objNode.Content, selKey, selSeq) + + mapNode.Content = append(mapNode.Content, aliasNode, objNode) + } + } + + mapping.Content = append(mapping.Content, keyNode, mapNode) +} diff --git a/internal/manifest/write_test.go b/internal/manifest/write_test.go index c447d97..101a2a1 100644 --- a/internal/manifest/write_test.go +++ b/internal/manifest/write_test.go @@ -14,8 +14,8 @@ func TestWriteRoundTrip(t *testing.T) { Description: "Test round-trip.", License: "MIT", Skills: []string{"./skills/one", "./skills/two"}, - Dependencies: map[string]string{ - "dep-a": "github.com/org/repo-a@v1.0.0", + Dependencies: map[string]DependencySpec{ + "dep-a": {URL: "github.com/org/repo-a@v1.0.0"}, }, Metadata: map[string]string{ "author": "test", @@ -52,8 +52,8 @@ func TestWriteRoundTrip(t *testing.T) { t.Errorf("Skills[%d]: got %q, want %q", i, parsed.Skills[i], s) } } - if parsed.Dependencies["dep-a"] != original.Dependencies["dep-a"] { - t.Errorf("Dependencies[dep-a]: got %q, want %q", parsed.Dependencies["dep-a"], original.Dependencies["dep-a"]) + if parsed.Dependencies["dep-a"].URL != original.Dependencies["dep-a"].URL { + t.Errorf("Dependencies[dep-a]: got %q, want %q", parsed.Dependencies["dep-a"].URL, original.Dependencies["dep-a"].URL) } if parsed.Metadata["author"] != original.Metadata["author"] { t.Errorf("Metadata[author]: got %q, want %q", parsed.Metadata["author"], original.Metadata["author"]) @@ -124,10 +124,10 @@ func TestWriteMapKeyOrder(t *testing.T) { SchemaVersion: 1, Name: "key-order", Skills: []string{"./skill"}, - Dependencies: map[string]string{ - "charlie": "github.com/org/charlie@v1.0.0", - "alpha": "github.com/org/alpha@v1.0.0", - "bravo": "github.com/org/bravo@v1.0.0", + Dependencies: map[string]DependencySpec{ + "charlie": {URL: "github.com/org/charlie@v1.0.0"}, + "alpha": {URL: "github.com/org/alpha@v1.0.0"}, + "bravo": {URL: "github.com/org/bravo@v1.0.0"}, }, Metadata: map[string]string{ "zebra": "z", @@ -173,6 +173,93 @@ func (errWriter) Write([]byte) (int, error) { return 0, fmt.Errorf("simulated write failure") } +func TestWriteRoundTripStructuredDeps(t *testing.T) { + original := &Manifest{ + SchemaVersion: 1, + Name: "structured-round-trip", + Skills: []string{"./skills/one"}, + Dependencies: map[string]DependencySpec{ + "acme": { + URL: "github.com/acme/skills@v1.0.0", + Select: []string{"skills/docx", "skills/pdf"}, + }, + }, + } + + var buf bytes.Buffer + if err := Write(original, &buf); err != nil { + t.Fatalf("Write failed: %v", err) + } + + output := buf.String() + if !strings.Contains(output, "url:") { + t.Error("Structured dep should contain 'url:' key") + } + if !strings.Contains(output, "select:") { + t.Error("Structured dep should contain 'select:' key") + } + + parsed, err := Parse(&buf) + if err != nil { + t.Fatalf("Parse failed: %v", err) + } + + dep := parsed.Dependencies["acme"] + if dep.URL != "github.com/acme/skills@v1.0.0" { + t.Errorf("URL: got %q, want %q", dep.URL, "github.com/acme/skills@v1.0.0") + } + if len(dep.Select) != 2 { + t.Fatalf("Select length: got %d, want 2", len(dep.Select)) + } + if dep.Select[0] != "skills/docx" || dep.Select[1] != "skills/pdf" { + t.Errorf("Select: got %v, want [skills/docx skills/pdf]", dep.Select) + } +} + +func TestWriteRoundTripMixedDeps(t *testing.T) { + original := &Manifest{ + SchemaVersion: 1, + Name: "mixed-round-trip", + Skills: []string{"./skills/one"}, + Dependencies: map[string]DependencySpec{ + "simple": {URL: "github.com/org/simple@v1.0.0"}, + "structured": { + URL: "github.com/org/structured@v2.0.0", + Select: []string{"skills/a"}, + }, + }, + } + + var buf bytes.Buffer + if err := Write(original, &buf); err != nil { + t.Fatalf("Write failed: %v", err) + } + + output := buf.String() + // Simple dep should be a scalar value + if !strings.Contains(output, "simple: github.com/org/simple@v1.0.0") { + t.Errorf("Simple dep should be scalar in output:\n%s", output) + } + + parsed, err := Parse(&buf) + if err != nil { + t.Fatalf("Parse failed: %v", err) + } + + if parsed.Dependencies["simple"].URL != "github.com/org/simple@v1.0.0" { + t.Errorf("Simple dep URL mismatch") + } + if len(parsed.Dependencies["simple"].Select) != 0 { + t.Errorf("Simple dep should have no Select") + } + if parsed.Dependencies["structured"].URL != "github.com/org/structured@v2.0.0" { + t.Errorf("Structured dep URL mismatch") + } + if len(parsed.Dependencies["structured"].Select) != 1 { + t.Errorf("Structured dep should have 1 Select entry") + } +} + func TestWriteError(t *testing.T) { m := &Manifest{ SchemaVersion: 1, diff --git a/internal/resolve/depurl.go b/internal/resolve/depurl.go index 82d9948..5d7bc50 100644 --- a/internal/resolve/depurl.go +++ b/internal/resolve/depurl.go @@ -38,7 +38,7 @@ const maxCommitSHALength = 64 // DepURL represents a parsed dependency URL from craft.yaml. type DepURL struct { - // Raw is the original URL string (e.g., "github.com/example/skills@v1.0.0"). + // Raw preserves the original (fragment-stripped) URL for diagnostic use and test verification. Raw string // Host is the registry host (e.g., "github.com"). @@ -60,6 +60,10 @@ type DepURL struct { // RefType identifies the kind of reference (tag, commit, branch). RefType RefType + + // Subpath is the optional subpath fragment (e.g., "skills/docx"). + // Parsed from #fragment syntax: host/org/repo@ref#subpath + Subpath string } // ParseDepURL parses a dependency URL string into its components. @@ -70,6 +74,13 @@ type DepURL struct { // // Returns an error if the URL does not match any expected format. func ParseDepURL(raw string) (*DepURL, error) { + // Extract optional #subpath fragment before any other parsing. + var subpath string + if hashIdx := strings.Index(raw, "#"); hashIdx >= 0 { + subpath = normalizeSubpath(raw[hashIdx+1:]) + raw = raw[:hashIdx] + } + atIdx := strings.Index(raw, "@") if atIdx < 0 { return nil, fmt.Errorf("invalid dependency URL %q: missing '@' — expected host/org/repo@ where ref is vX.Y.Z, a commit SHA, or branch:", raw) @@ -88,10 +99,11 @@ func ParseDepURL(raw string) (*DepURL, error) { } d := &DepURL{ - Raw: raw, - Host: matches[1], - Org: matches[2], - Repo: matches[3], + Raw: raw, + Host: matches[1], + Org: matches[2], + Repo: matches[3], + Subpath: subpath, } if strings.HasPrefix(ref, "branch:") { @@ -169,10 +181,20 @@ func (d *DepURL) WithVersion(version string) string { return d.PackageIdentity() + "@v" + version } -// String returns the raw dependency URL, or reconstructs it from components. +// String returns the dependency URL, reconstructed from components. func (d *DepURL) String() string { - if d.Raw != "" { - return d.Raw + s := d.PackageIdentity() + "@" + d.RefString() + if d.Subpath != "" { + s += "#" + d.Subpath } - return d.PackageIdentity() + "@" + d.RefString() + return s +} + +// normalizeSubpath strips leading "./" and trailing "/" from a subpath. +// Returns empty string for empty or whitespace-only input. +func normalizeSubpath(s string) string { + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "./") + s = strings.TrimSuffix(s, "/") + return s } diff --git a/internal/resolve/depurl_test.go b/internal/resolve/depurl_test.go index 45fc7d7..6744a07 100644 --- a/internal/resolve/depurl_test.go +++ b/internal/resolve/depurl_test.go @@ -168,6 +168,85 @@ func TestParseDepURL(t *testing.T) { RefType: RefTypeBranch, }, }, + // Subpath tests + { + name: "tag with subpath", + input: "github.com/acme/skills@v1.0.0#skills/docx", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "skills/docx", + }, + }, + { + name: "commit with subpath", + input: "github.com/acme/tools@abc1234#tools/helper", + want: &DepURL{ + Raw: "github.com/acme/tools@abc1234", + Host: "github.com", + Org: "acme", + Repo: "tools", + Ref: "abc1234", + RefType: RefTypeCommit, + Subpath: "tools/helper", + }, + }, + { + name: "branch with subpath", + input: "github.com/acme/tools@branch:main#tools/x", + want: &DepURL{ + Raw: "github.com/acme/tools@branch:main", + Host: "github.com", + Org: "acme", + Repo: "tools", + Ref: "main", + RefType: RefTypeBranch, + Subpath: "tools/x", + }, + }, + { + name: "nested subpath", + input: "github.com/acme/skills@v2.0.0#skills/nested/deep/docx", + want: &DepURL{ + Raw: "github.com/acme/skills@v2.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "2.0.0", + RefType: RefTypeTag, + Subpath: "skills/nested/deep/docx", + }, + }, + { + name: "subpath with leading dot-slash normalized", + input: "github.com/acme/skills@v1.0.0#./skills/docx", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "skills/docx", + }, + }, + { + name: "empty fragment means no subpath", + input: "github.com/acme/skills@v1.0.0#", + want: &DepURL{ + Raw: "github.com/acme/skills@v1.0.0", + Host: "github.com", + Org: "acme", + Repo: "skills", + Version: "1.0.0", + RefType: RefTypeTag, + Subpath: "", + }, + }, // Error cases { name: "missing ref (no @)", @@ -243,7 +322,7 @@ func TestParseDepURL(t *testing.T) { if err != nil { t.Fatalf("ParseDepURL(%q) error: %v", tt.input, err) } - if got.Raw != tt.want.Raw || got.Host != tt.want.Host || got.Org != tt.want.Org || got.Repo != tt.want.Repo || got.Version != tt.want.Version || got.Ref != tt.want.Ref || got.RefType != tt.want.RefType { + if got.Raw != tt.want.Raw || got.Host != tt.want.Host || got.Org != tt.want.Org || got.Repo != tt.want.Repo || got.Version != tt.want.Version || got.Ref != tt.want.Ref || got.RefType != tt.want.RefType || got.Subpath != tt.want.Subpath { t.Errorf("ParseDepURL(%q) = %+v, want %+v", tt.input, got, tt.want) } }) @@ -341,3 +420,30 @@ func TestDepURLMethodsBranch(t *testing.T) { t.Errorf("String() = %q, want %q", got, "github.com/acme/tools@branch:main") } } + +func TestDepURLSubpathMethods(t *testing.T) { + d, err := ParseDepURL("github.com/acme/skills@v1.0.0#skills/docx") + if err != nil { + t.Fatalf("ParseDepURL error: %v", err) + } + + // PackageIdentity excludes subpath + if got := d.PackageIdentity(); got != "github.com/acme/skills" { + t.Errorf("PackageIdentity() = %q, want %q", got, "github.com/acme/skills") + } + + // String includes subpath + if got := d.String(); got != "github.com/acme/skills@v1.0.0#skills/docx" { + t.Errorf("String() = %q, want %q", got, "github.com/acme/skills@v1.0.0#skills/docx") + } + + // WithVersion drops subpath + if got := d.WithVersion("2.0.0"); got != "github.com/acme/skills@v2.0.0" { + t.Errorf("WithVersion() = %q, want %q", got, "github.com/acme/skills@v2.0.0") + } + + // GitRef is unaffected by subpath + if got := d.GitRef(); got != "v1.0.0" { + t.Errorf("GitRef() = %q, want %q", got, "v1.0.0") + } +} diff --git a/internal/resolve/resolver.go b/internal/resolve/resolver.go index 1d66721..d9bc9d3 100644 --- a/internal/resolve/resolver.go +++ b/internal/resolve/resolver.go @@ -163,6 +163,26 @@ func (r *Resolver) Resolve(m *manifest.Manifest, opts ResolveOptions) (*ResolveR } } } + + // Merge Select lists from all entries referencing this package. + // Empty select in any entry → all skills (nil). + if best, ok := selected[identity]; ok { + var mergedSelect []string + allSkills := false + for _, dep := range deps { + if len(dep.Select) == 0 { + allSkills = true + break + } + mergedSelect = append(mergedSelect, dep.Select...) + } + if allSkills { + best.Select = nil + } else { + best.Select = deduplicateStrings(mergedSelect) + } + selected[identity] = best + } } // Re-collect transitive deps for packages where MVS selected @@ -279,8 +299,8 @@ func (r *Resolver) collectDeps(m *manifest.Manifest, parentID, source string, gr var allDeps []ResolvedDep - for alias, depURL := range m.Dependencies { - parsed, err := ParseDepURL(depURL) + for alias, depSpec := range m.Dependencies { + parsed, err := ParseDepURL(depSpec.URL) if err != nil { return nil, fmt.Errorf("invalid dependency URL for %q: %w", alias, err) } @@ -289,10 +309,11 @@ func (r *Resolver) collectDeps(m *manifest.Manifest, parentID, source string, gr graph.AddEdge(parentID, identity) dep := ResolvedDep{ - URL: depURL, + URL: depSpec.URL, Alias: alias, Source: source, RefType: parsed.RefType, + Select: depSpec.Select, } allDeps = append(allDeps, dep) @@ -309,7 +330,7 @@ func (r *Resolver) collectDeps(m *manifest.Manifest, parentID, source string, gr commitSHA, err := r.fetcher.ResolveRef(cloneURL, parsed.GitRef()) if err != nil { - return nil, fmt.Errorf("resolving %s: %w", depURL, err) + return nil, fmt.Errorf("resolving %s: %w", depSpec.URL, err) } files, err := r.fetcher.ReadFiles(cloneURL, commitSHA, []string{"craft.yaml"}) @@ -321,11 +342,11 @@ func (r *Resolver) collectDeps(m *manifest.Manifest, parentID, source string, gr if craftYAML, ok := files["craft.yaml"]; ok { depManifest, err := manifest.Parse(bytes.NewReader(craftYAML)) if err != nil { - return nil, fmt.Errorf("parsing craft.yaml from %s: %w", depURL, err) + return nil, fmt.Errorf("parsing craft.yaml from %s: %w", depSpec.URL, err) } if len(depManifest.Dependencies) > 0 { - transitive, err := r.collectDeps(depManifest, identity, depURL, graph, opts, visited, depth+1) + transitive, err := r.collectDeps(depManifest, identity, depSpec.URL, graph, opts, visited, depth+1) if err != nil { return nil, err } @@ -345,7 +366,7 @@ func (r *Resolver) resolveOne(dep ResolvedDep, opts ResolveOptions) (ResolvedDep } // Check pinfile reuse — skip for branch deps (must always re-resolve) - if opts.ExistingPinfile != nil && !opts.ForceResolve[dep.URL] && parsed.RefType != RefTypeBranch { + if opts.ExistingPinfile != nil && !opts.ForceResolve[dep.URL] && parsed.RefType != RefTypeBranch && len(dep.Select) == 0 { if entry, ok := opts.ExistingPinfile.Resolved[dep.URL]; ok { dep.Commit = entry.Commit dep.Integrity = entry.Integrity @@ -365,11 +386,22 @@ func (r *Resolver) resolveOne(dep ResolvedDep, opts ResolveOptions) (ResolvedDep dep.Commit = commitSHA // Discover skills and compute integrity - skillNames, skillPaths, skillFiles, err := r.discoverSkillsForDep(cloneURL, commitSHA) + forceAutoDiscover := len(dep.Select) > 0 + skillNames, skillPaths, skillFiles, err := r.discoverSkillsForDep(cloneURL, commitSHA, forceAutoDiscover) if err != nil { return dep, err } + dep.AllSkillPaths = skillPaths + + // Apply select filter if specified + if len(dep.Select) > 0 { + skillNames, skillPaths, skillFiles, err = filterBySelect(skillNames, skillPaths, skillFiles, dep.Select) + if err != nil { + return dep, fmt.Errorf("filtering skills for %s: %w", dep.URL, err) + } + } + dep.Skills = skillNames dep.SkillPaths = skillPaths dep.Integrity = integrity.Digest(skillFiles) @@ -378,18 +410,22 @@ func (r *Resolver) resolveOne(dep ResolvedDep, opts ResolveOptions) (ResolvedDep } // discoverSkillsForDep finds skills in a dependency, trying craft.yaml first, -// then falling back to auto-discovery. -func (r *Resolver) discoverSkillsForDep(cloneURL, commitSHA string) (names []string, dirs []string, files map[string][]byte, err error) { - // Try reading craft.yaml - craftFiles, readErr := r.fetcher.ReadFiles(cloneURL, commitSHA, []string{"craft.yaml"}) - if readErr == nil { - if craftYAML, ok := craftFiles["craft.yaml"]; ok { - depManifest, parseErr := manifest.Parse(bytes.NewReader(craftYAML)) - if parseErr != nil { - return nil, nil, nil, fmt.Errorf("parsing craft.yaml in %s: %w", cloneURL, parseErr) - } - if len(depManifest.Skills) > 0 { - return r.discoverFromManifestSkills(cloneURL, commitSHA, depManifest.Skills) +// then falling back to auto-discovery. When forceAutoDiscover is true, the +// manifest's skills export list is bypassed — this ensures consumer select +// can reach skills not in the package's export list (FR-006). +func (r *Resolver) discoverSkillsForDep(cloneURL, commitSHA string, forceAutoDiscover bool) (names []string, dirs []string, files map[string][]byte, err error) { + if !forceAutoDiscover { + // Try reading craft.yaml + craftFiles, readErr := r.fetcher.ReadFiles(cloneURL, commitSHA, []string{"craft.yaml"}) + if readErr == nil { + if craftYAML, ok := craftFiles["craft.yaml"]; ok { + depManifest, parseErr := manifest.Parse(bytes.NewReader(craftYAML)) + if parseErr != nil { + return nil, nil, nil, fmt.Errorf("parsing craft.yaml in %s: %w", cloneURL, parseErr) + } + if len(depManifest.Skills) > 0 { + return r.discoverFromManifestSkills(cloneURL, commitSHA, depManifest.Skills) + } } } } @@ -571,3 +607,68 @@ func IsInfraFile(path string) bool { return false } + +// filterBySelect filters discovered skills to only those matching the select +// paths. Returns an error if any select path has no matching skill. +func filterBySelect(names, dirs []string, files map[string][]byte, selectPaths []string) ([]string, []string, map[string][]byte, error) { + if len(selectPaths) == 0 { + return names, dirs, files, nil + } + + // Normalize select paths for comparison + normalized := make([]string, len(selectPaths)) + for i, s := range selectPaths { + s = strings.TrimPrefix(s, "./") + s = strings.TrimSuffix(s, "/") + normalized[i] = s + } + + // Build lookup from dir → index for matching + dirIndex := make(map[string]int, len(dirs)) + for i, d := range dirs { + dirIndex[d] = i + } + + var filteredNames []string + var filteredDirs []string + filteredFiles := make(map[string][]byte) + + for _, sel := range normalized { + idx, ok := dirIndex[sel] + if !ok { + return nil, nil, nil, fmt.Errorf("selected path %q does not match any skill in the package (available: %s)", + sel, strings.Join(dirs, ", ")) + } + filteredNames = append(filteredNames, names[idx]) + filteredDirs = append(filteredDirs, dirs[idx]) + + // Include files under this skill directory + prefix := sel + "/" + // Safety: empty sel should be rejected by manifest validation; + // guard against inclusion of all files if validation is bypassed. + if sel == "" { + prefix = "" + } + for path, content := range files { + if prefix == "" || strings.HasPrefix(path, prefix) { + filteredFiles[path] = content + } + } + } + + return filteredNames, filteredDirs, filteredFiles, nil +} + +// deduplicateStrings returns a sorted, deduplicated copy of the input slice. +func deduplicateStrings(ss []string) []string { + seen := make(map[string]bool, len(ss)) + var result []string + for _, s := range ss { + if !seen[s] { + seen[s] = true + result = append(result, s) + } + } + sort.Strings(result) + return result +} diff --git a/internal/resolve/resolver_test.go b/internal/resolve/resolver_test.go index 52e3cce..de60329 100644 --- a/internal/resolve/resolver_test.go +++ b/internal/resolve/resolver_test.go @@ -47,7 +47,7 @@ func TestResolveSingleDep(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{"skills": "github.com/org/skills@v1.0.0"}, + Dependencies: map[string]manifest.DependencySpec{"skills": {URL: "github.com/org/skills@v1.0.0"}}, } result, err := resolver.Resolve(m, ResolveOptions{}) @@ -95,9 +95,9 @@ func TestResolveMVSDiamond(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "root", - Dependencies: map[string]string{ - "b": "github.com/org/b@v1.0.0", - "d": "github.com/org/d@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "b": {URL: "github.com/org/b@v1.0.0"}, + "d": {URL: "github.com/org/d@v1.0.0"}, }, } @@ -143,7 +143,7 @@ func TestResolveCycleDetection(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "root", - Dependencies: map[string]string{"a": "github.com/org/a@v1.0.0"}, + Dependencies: map[string]manifest.DependencySpec{"a": {URL: "github.com/org/a@v1.0.0"}}, } _, err := resolver.Resolve(m, ResolveOptions{}) @@ -165,9 +165,9 @@ func TestResolveSameNameSkillsAllowed(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "root", - Dependencies: map[string]string{ - "a": "github.com/org/a@v1.0.0", - "b": "github.com/org/b@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "a": {URL: "github.com/org/a@v1.0.0"}, + "b": {URL: "github.com/org/b@v1.0.0"}, }, } @@ -199,7 +199,7 @@ func TestResolvePinfileReuse(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{"skills": "github.com/org/skills@v1.0.0"}, + Dependencies: map[string]manifest.DependencySpec{"skills": {URL: "github.com/org/skills@v1.0.0"}}, } result, err := resolver.Resolve(m, ResolveOptions{ExistingPinfile: existing}) @@ -235,7 +235,7 @@ func TestResolveForceResolve(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{"skills": "github.com/org/skills@v1.0.0"}, + Dependencies: map[string]manifest.DependencySpec{"skills": {URL: "github.com/org/skills@v1.0.0"}}, } result, err := resolver.Resolve(m, ResolveOptions{ @@ -299,7 +299,7 @@ func TestResolveDepthLimit(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "root", - Dependencies: map[string]string{"dep0": "github.com/org/dep0@v1.0.0"}, + Dependencies: map[string]manifest.DependencySpec{"dep0": {URL: "github.com/org/dep0@v1.0.0"}}, } _, err := resolver.Resolve(m, ResolveOptions{}) @@ -350,9 +350,9 @@ func TestResolveMVSTransitiveDepsReCollection(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "root", - Dependencies: map[string]string{ - "a": "github.com/org/a@v1.0.0", - "b": "github.com/org/b@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "a": {URL: "github.com/org/a@v1.0.0"}, + "b": {URL: "github.com/org/b@v1.0.0"}, }, } @@ -418,7 +418,7 @@ func TestResolveBranchDep(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{"tools": "github.com/acme/tools@branch:main"}, + Dependencies: map[string]manifest.DependencySpec{"tools": {URL: "github.com/acme/tools@branch:main"}}, } result, err := resolver.Resolve(m, ResolveOptions{}) @@ -456,7 +456,7 @@ func TestResolveCommitDep(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{"tools": "github.com/acme/tools@abc1234def567890abc1234def567890abc1234d"}, + Dependencies: map[string]manifest.DependencySpec{"tools": {URL: "github.com/acme/tools@abc1234def567890abc1234def567890abc1234d"}}, } result, err := resolver.Resolve(m, ResolveOptions{}) @@ -499,9 +499,9 @@ func TestResolveMixedRefTypeConflict(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{ - "tools": "github.com/acme/tools@v1.0.0", - "b": "github.com/org/b@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@v1.0.0"}, + "b": {URL: "github.com/org/b@v1.0.0"}, }, } @@ -528,9 +528,9 @@ func TestResolveSameBranchMerge(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{ - "tools": "github.com/acme/tools@branch:main", - "b": "github.com/org/b@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@branch:main"}, + "b": {URL: "github.com/org/b@v1.0.0"}, }, } @@ -562,9 +562,9 @@ func TestResolveDifferentBranchConflict(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{ - "tools": "github.com/acme/tools@branch:main", - "b": "github.com/org/b@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@branch:main"}, + "b": {URL: "github.com/org/b@v1.0.0"}, }, } @@ -596,7 +596,7 @@ func TestResolveBranchDepBypassesPinfileCache(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{"tools": "github.com/acme/tools@branch:main"}, + Dependencies: map[string]manifest.DependencySpec{"tools": {URL: "github.com/acme/tools@branch:main"}}, } result, err := resolver.Resolve(m, ResolveOptions{ExistingPinfile: existingPinfile}) @@ -638,9 +638,9 @@ func TestResolveConflictingCommitSHAs(t *testing.T) { resolver := NewResolver(mock) m := &manifest.Manifest{ Name: "test", - Dependencies: map[string]string{ - "b": "github.com/org/b@v1.0.0", - "c": "github.com/org/c@v1.0.0", + Dependencies: map[string]manifest.DependencySpec{ + "b": {URL: "github.com/org/b@v1.0.0"}, + "c": {URL: "github.com/org/c@v1.0.0"}, }, } @@ -652,3 +652,307 @@ func TestResolveConflictingCommitSHAs(t *testing.T) { t.Errorf("Error = %q, want commit SHA conflict message", err.Error()) } } + +// --- Phase 3: Select/Filter tests --- + +func TestFilterBySelect(t *testing.T) { + names := []string{"docx", "pdf", "xlsx"} + dirs := []string{"skills/docx", "skills/pdf", "skills/xlsx"} + files := map[string][]byte{ + "skills/docx/SKILL.md": []byte("docx"), + "skills/docx/prompt.md": []byte("docx prompt"), + "skills/pdf/SKILL.md": []byte("pdf"), + "skills/xlsx/SKILL.md": []byte("xlsx"), + } + + // Select 2 of 3 + n, d, f, err := filterBySelect(names, dirs, files, []string{"skills/docx", "skills/pdf"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(n) != 2 { + t.Errorf("expected 2 names, got %d", len(n)) + } + if len(d) != 2 { + t.Errorf("expected 2 dirs, got %d", len(d)) + } + if n[0] != "docx" || n[1] != "pdf" { + t.Errorf("names = %v, want [docx pdf]", n) + } + // Verify files only include docx and pdf + if _, ok := f["skills/xlsx/SKILL.md"]; ok { + t.Error("filtered files should not include xlsx") + } + if _, ok := f["skills/docx/SKILL.md"]; !ok { + t.Error("filtered files should include docx/SKILL.md") + } + if _, ok := f["skills/docx/prompt.md"]; !ok { + t.Error("filtered files should include docx/prompt.md") + } + + // Select with ./ prefix normalization + n2, _, _, err := filterBySelect(names, dirs, files, []string{"./skills/docx"}) + if err != nil { + t.Fatalf("unexpected error with ./ prefix: %v", err) + } + if len(n2) != 1 || n2[0] != "docx" { + t.Errorf("normalized select: names = %v, want [docx]", n2) + } + + // Select with trailing / normalization + n3, _, _, err := filterBySelect(names, dirs, files, []string{"skills/pdf/"}) + if err != nil { + t.Fatalf("unexpected error with trailing /: %v", err) + } + if len(n3) != 1 || n3[0] != "pdf" { + t.Errorf("normalized select: names = %v, want [pdf]", n3) + } + + // Select non-existent path → error + _, _, _, err = filterBySelect(names, dirs, files, []string{"skills/nonexistent"}) + if err == nil { + t.Error("expected error for non-existent path") + } + if err != nil && !strings.Contains(err.Error(), "does not match any skill") { + t.Errorf("error = %q, want 'does not match' message", err.Error()) + } + + // Empty select = all + n4, d4, f4, err := filterBySelect(names, dirs, files, nil) + if err != nil { + t.Fatalf("unexpected error for nil select: %v", err) + } + if len(n4) != 3 { + t.Errorf("empty select should return all: got %d names", len(n4)) + } + if len(d4) != 3 { + t.Errorf("empty select should return all: got %d dirs", len(d4)) + } + if len(f4) != len(files) { + t.Errorf("empty select should return all files: got %d, want %d", len(f4), len(files)) + } +} + +func TestFilterBySelectPrefixCollision(t *testing.T) { + names := []string{"doc", "docx"} + dirs := []string{"skills/doc", "skills/docx"} + files := map[string][]byte{ + "skills/doc/SKILL.md": []byte("doc"), + "skills/doc/prompt.md": []byte("doc prompt"), + "skills/docx/SKILL.md": []byte("docx"), + "skills/docx/prompt.md": []byte("docx prompt"), + "skills/docx/examples.txt": []byte("docx examples"), + } + + filteredNames, filteredDirs, filteredFiles, err := filterBySelect(names, dirs, files, []string{"skills/doc"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(filteredNames) != 1 || filteredNames[0] != "doc" { + t.Errorf("filteredNames = %v, want [doc]", filteredNames) + } + if len(filteredDirs) != 1 || filteredDirs[0] != "skills/doc" { + t.Errorf("filteredDirs = %v, want [skills/doc]", filteredDirs) + } + if len(filteredFiles) != 2 { + t.Fatalf("filteredFiles length = %d, want 2", len(filteredFiles)) + } + if _, ok := filteredFiles["skills/doc/SKILL.md"]; !ok { + t.Error("filtered files should include skills/doc/SKILL.md") + } + if _, ok := filteredFiles["skills/doc/prompt.md"]; !ok { + t.Error("filtered files should include skills/doc/prompt.md") + } + if _, ok := filteredFiles["skills/docx/SKILL.md"]; ok { + t.Error("filtered files should not include skills/docx/SKILL.md") + } + if _, ok := filteredFiles["skills/docx/prompt.md"]; ok { + t.Error("filtered files should not include skills/docx/prompt.md") + } +} + +func setupMultiSkillDep(mock *fetch.MockFetcher, identity, version, commitSHA string) { + cloneURL := "https://" + identity + ".git" + tag := "v" + version + mock.Refs[cloneURL+":"+tag] = commitSHA + mock.Trees[cloneURL+":"+commitSHA] = []string{ + "skills/docx/SKILL.md", + "skills/pdf/SKILL.md", + "skills/xlsx/SKILL.md", + } + mock.Files[cloneURL+":"+commitSHA+":skills/docx/SKILL.md"] = []byte("---\nname: docx\n---\n") + mock.Files[cloneURL+":"+commitSHA+":skills/pdf/SKILL.md"] = []byte("---\nname: pdf\n---\n") + mock.Files[cloneURL+":"+commitSHA+":skills/xlsx/SKILL.md"] = []byte("---\nname: xlsx\n---\n") +} + +func TestResolveEmptySelect(t *testing.T) { + mock := newTestFetcher() + setupMultiSkillDep(mock, "github.com/acme/tools", "1.0.0", "abc123") + + resolver := NewResolver(mock) + m := &manifest.Manifest{ + Name: "test", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@v1.0.0"}, + }, + } + + result, err := resolver.Resolve(m, ResolveOptions{}) + if err != nil { + t.Fatalf("Resolve error: %v", err) + } + if len(result.Resolved) != 1 { + t.Fatalf("Expected 1 resolved, got %d", len(result.Resolved)) + } + dep := result.Resolved[0] + if len(dep.Skills) != 3 { + t.Errorf("Empty select should return all skills, got %d: %v", len(dep.Skills), dep.Skills) + } + if len(dep.AllSkillPaths) != 3 { + t.Errorf("AllSkillPaths should have all 3, got %d: %v", len(dep.AllSkillPaths), dep.AllSkillPaths) + } +} + +func TestResolveSelectNotFound(t *testing.T) { + mock := newTestFetcher() + setupMultiSkillDep(mock, "github.com/acme/tools", "1.0.0", "abc123") + + resolver := NewResolver(mock) + m := &manifest.Manifest{ + Name: "test", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@v1.0.0", Select: []string{"skills/nonexistent"}}, + }, + } + + _, err := resolver.Resolve(m, ResolveOptions{}) + if err == nil { + t.Fatal("Expected error for non-existent select path") + } + if !strings.Contains(err.Error(), "does not match any skill") { + t.Errorf("Error = %q, want 'does not match' message", err.Error()) + } +} + +func TestResolveSelectMerge(t *testing.T) { + mock := newTestFetcher() + setupMultiSkillDep(mock, "github.com/acme/tools", "1.0.0", "abc123") + + // Package B also depends on tools with a different select + bClone := "https://github.com/org/b.git" + mock.Refs[bClone+":v1.0.0"] = "bbb" + mock.Trees[bClone+":bbb"] = []string{"skills/b-skill/SKILL.md"} + mock.Files[bClone+":bbb:skills/b-skill/SKILL.md"] = []byte("---\nname: b-skill\n---\n") + mock.Files[bClone+":bbb:craft.yaml"] = []byte("schema_version: 1\nname: b\nversion: 1.0.0\nskills:\n - ./skills/b-skill\ndependencies:\n tools: github.com/acme/tools@v1.0.0\n") + + resolver := NewResolver(mock) + m := &manifest.Manifest{ + Name: "test", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@v1.0.0", Select: []string{"skills/docx"}}, + "b": {URL: "github.com/org/b@v1.0.0"}, + }, + } + + result, err := resolver.Resolve(m, ResolveOptions{}) + if err != nil { + t.Fatalf("Resolve error: %v", err) + } + + // Find the tools dep — B has empty select, so the merge should result + // in all skills (empty select wins). + for _, dep := range result.Resolved { + if strings.Contains(dep.URL, "acme/tools") { + if len(dep.Skills) != 3 { + t.Errorf("Merged select should include all skills (empty select from transitive), got %d: %v", len(dep.Skills), dep.Skills) + } + return + } + } + t.Fatal("tools dep not found in resolved") +} + +func TestResolveSelectMergeUnion(t *testing.T) { + mock := newTestFetcher() + setupMultiSkillDep(mock, "github.com/acme/tools", "1.0.0", "abc123") + setupMultiSkillDep(mock, "github.com/acme/tools", "2.0.0", "def456") + + // B depends on tools@v2.0.0 with select [skills/xlsx] + bClone := "https://github.com/org/b.git" + mock.Refs[bClone+":v1.0.0"] = "bbb" + mock.Trees[bClone+":bbb"] = []string{"skills/b-skill/SKILL.md"} + mock.Files[bClone+":bbb:skills/b-skill/SKILL.md"] = []byte("---\nname: b-skill\n---\n") + mock.Files[bClone+":bbb:craft.yaml"] = []byte("schema_version: 1\nname: b\nversion: 1.0.0\nskills:\n - ./skills/b-skill\ndependencies:\n tools:\n url: github.com/acme/tools@v2.0.0\n select:\n - skills/xlsx\n") + + resolver := NewResolver(mock) + m := &manifest.Manifest{ + Name: "test", + Dependencies: map[string]manifest.DependencySpec{ + "tools": {URL: "github.com/acme/tools@v1.0.0", Select: []string{"skills/docx"}}, + "b": {URL: "github.com/org/b@v1.0.0"}, + }, + } + + result, err := resolver.Resolve(m, ResolveOptions{}) + if err != nil { + t.Fatalf("Resolve error: %v", err) + } + + for _, dep := range result.Resolved { + if strings.Contains(dep.URL, "acme/tools") { + // MVS picks v2.0.0 (higher). Merged select should be union of + // [skills/docx] and [skills/xlsx]. + if len(dep.Skills) != 2 { + t.Errorf("Merged select should have 2 skills (docx+xlsx), got %d: %v", len(dep.Skills), dep.Skills) + } + return + } + } + t.Fatal("tools dep not found in resolved") +} + +func TestResolveSelectOverridesExports(t *testing.T) { + mock := newTestFetcher() + + // Package with craft.yaml that exports only skills/public, + // but also has skills/internal available via auto-discovery. + cloneURL := "https://github.com/acme/restricted.git" + mock.Refs[cloneURL+":v1.0.0"] = "abc123" + mock.Trees[cloneURL+":abc123"] = []string{ + "craft.yaml", + "skills/public/SKILL.md", + "skills/internal/SKILL.md", + } + mock.Files[cloneURL+":abc123:craft.yaml"] = []byte("schema_version: 1\nname: restricted\nversion: 1.0.0\nskills:\n - ./skills/public\n") + mock.Files[cloneURL+":abc123:skills/public/SKILL.md"] = []byte("---\nname: public-skill\n---\n") + mock.Files[cloneURL+":abc123:skills/internal/SKILL.md"] = []byte("---\nname: internal-skill\n---\n") + + resolver := NewResolver(mock) + + // Consumer selects skills/internal — which is NOT in the package's exports + m := &manifest.Manifest{ + Name: "test", + Dependencies: map[string]manifest.DependencySpec{ + "restricted": {URL: "github.com/acme/restricted@v1.0.0", Select: []string{"skills/internal"}}, + }, + } + + result, err := resolver.Resolve(m, ResolveOptions{}) + if err != nil { + t.Fatalf("Resolve error: %v", err) + } + if len(result.Resolved) != 1 { + t.Fatalf("Expected 1 resolved, got %d", len(result.Resolved)) + } + dep := result.Resolved[0] + if len(dep.Skills) != 1 || dep.Skills[0] != "internal-skill" { + t.Errorf("Select should override exports, got skills: %v", dep.Skills) + } + if len(dep.SkillPaths) != 1 || dep.SkillPaths[0] != "skills/internal" { + t.Errorf("SkillPaths should be [skills/internal], got: %v", dep.SkillPaths) + } + // AllSkillPaths should include both (full auto-discovery) + if len(dep.AllSkillPaths) != 2 { + t.Errorf("AllSkillPaths should have 2 (both public and internal), got %d: %v", len(dep.AllSkillPaths), dep.AllSkillPaths) + } +} diff --git a/internal/resolve/types.go b/internal/resolve/types.go index 6502f50..5ce7f72 100644 --- a/internal/resolve/types.go +++ b/internal/resolve/types.go @@ -26,4 +26,11 @@ type ResolvedDep struct { // RefType identifies the kind of reference (tag, commit, branch). RefType RefType + + // Select is the subpath filter from the manifest (empty = all skills). + Select []string + + // AllSkillPaths records the full set of discovered skill paths before + // filtering, enabling new-skill detection without a second fetch pass. + AllSkillPaths []string } diff --git a/internal/ui/select.go b/internal/ui/select.go new file mode 100644 index 0000000..6b76de7 --- /dev/null +++ b/internal/ui/select.go @@ -0,0 +1,73 @@ +package ui + +import ( + "bufio" + "fmt" + "io" + "strconv" + "strings" +) + +// MultiSelect presents a numbered list of items and lets the user select +// which to include by entering numbers. Returns indices of selected items. +// If all are selected, returns nil (meaning "all"). +func MultiSelect(prompt string, items []string, w io.Writer, r io.Reader) ([]int, error) { + if len(items) == 0 { + return nil, nil + } + _, _ = fmt.Fprintln(w, prompt) + for i, item := range items { + _, _ = fmt.Fprintf(w, " [%d] %s\n", i+1, item) + } + _, _ = fmt.Fprint(w, "Enter numbers to include (e.g. 1,3,5), 'a' for all, or Enter for all: ") + + scanner := bufio.NewScanner(r) + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("reading selection: %w", err) + } + // EOF — treat as "all" + return nil, nil + } + + input := strings.TrimSpace(scanner.Text()) + + // Empty or "all" → select everything + if input == "" || strings.EqualFold(input, "a") || strings.EqualFold(input, "all") { + return nil, nil + } + + // Parse comma-separated numbers + parts := strings.Split(input, ",") + seen := make(map[int]bool) + var indices []int + for _, part := range parts { + s := strings.TrimSpace(part) + if s == "" { + continue + } + num, err := strconv.Atoi(s) + if err != nil { + return nil, fmt.Errorf("invalid number %q — enter comma-separated numbers (e.g. 1,3,5)", s) + } + if num < 1 || num > len(items) { + return nil, fmt.Errorf("number %d out of range (1–%d)", num, len(items)) + } + idx := num - 1 + if !seen[idx] { + seen[idx] = true + indices = append(indices, idx) + } + } + + if len(indices) == 0 { + return nil, nil + } + + // If all selected, return nil + if len(indices) == len(items) { + return nil, nil + } + + return indices, nil +} diff --git a/internal/ui/select_test.go b/internal/ui/select_test.go new file mode 100644 index 0000000..515704d --- /dev/null +++ b/internal/ui/select_test.go @@ -0,0 +1,142 @@ +package ui + +import ( + "bytes" + "strings" + "testing" +) + +func TestMultiSelectSpecificItems(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("1,3\n") + + items := []string{"alpha", "beta", "gamma", "delta"} + got, err := MultiSelect("Pick skills:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 2 || got[0] != 0 || got[1] != 2 { + t.Errorf("expected [0, 2], got %v", got) + } + + // Verify output contains numbered list + output := out.String() + if !strings.Contains(output, "[1] alpha") { + t.Errorf("expected numbered list in output, got:\n%s", output) + } + if !strings.Contains(output, "[4] delta") { + t.Errorf("expected [4] delta in output, got:\n%s", output) + } +} + +func TestMultiSelectAll(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("a\n") + + items := []string{"alpha", "beta", "gamma"} + got, err := MultiSelect("Pick:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != nil { + t.Errorf("expected nil (all), got %v", got) + } +} + +func TestMultiSelectEmpty(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("\n") + + items := []string{"alpha", "beta"} + got, err := MultiSelect("Pick:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != nil { + t.Errorf("expected nil (all), got %v", got) + } +} + +func TestMultiSelectSingle(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("1\n") + + items := []string{"alpha", "beta", "gamma"} + got, err := MultiSelect("Pick:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 1 || got[0] != 0 { + t.Errorf("expected [0], got %v", got) + } +} + +func TestMultiSelectOutOfRange(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("5\n") + + items := []string{"alpha", "beta"} + _, err := MultiSelect("Pick:", items, &out, r) + if err == nil { + t.Fatal("expected error for out-of-range number") + } + if !strings.Contains(err.Error(), "out of range") { + t.Errorf("expected out-of-range error, got: %v", err) + } +} + +func TestMultiSelectInvalidInput(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("abc\n") + + items := []string{"alpha", "beta"} + _, err := MultiSelect("Pick:", items, &out, r) + if err == nil { + t.Fatal("expected error for invalid input") + } + if !strings.Contains(err.Error(), "invalid number") { + t.Errorf("expected invalid number error, got: %v", err) + } +} + +func TestMultiSelectAllExplicit(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("all\n") + + items := []string{"alpha", "beta"} + got, err := MultiSelect("Pick:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != nil { + t.Errorf("expected nil (all), got %v", got) + } +} + +func TestMultiSelectAllIndicesReturnsNil(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("1,2,3\n") + + items := []string{"alpha", "beta", "gamma"} + got, err := MultiSelect("Pick:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != nil { + t.Errorf("expected nil when all items selected, got %v", got) + } +} + +func TestMultiSelectDuplicateNumbers(t *testing.T) { + var out bytes.Buffer + r := strings.NewReader("1,1,2\n") + + items := []string{"alpha", "beta", "gamma"} + got, err := MultiSelect("Pick:", items, &out, r) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 2 || got[0] != 0 || got[1] != 1 { + t.Errorf("expected [0, 1] (deduped), got %v", got) + } +} diff --git a/internal/validate/runner.go b/internal/validate/runner.go index bf0548d..672be10 100644 --- a/internal/validate/runner.go +++ b/internal/validate/runner.go @@ -264,13 +264,13 @@ func (r *Runner) checkPinfile(result *Result, m *manifest.Manifest) *pinfile.Pin // Each manifest dependency should have a pinfile entry depURLs := make(map[string]bool) - for _, url := range m.Dependencies { - depURLs[url] = true - if _, ok := p.Resolved[url]; !ok { + for _, dep := range m.Dependencies { + depURLs[dep.URL] = true + if _, ok := p.Resolved[dep.URL]; !ok { result.Errors = append(result.Errors, &Error{ Category: CategoryPinfile, Path: "craft.pin.yaml", - Field: url, + Field: dep.URL, Message: "manifest dependency has no matching pinfile entry", Suggestion: "Run 'craft install' to regenerate the pinfile", }) @@ -315,19 +315,19 @@ func (r *Runner) checkNameCollisions(result *Result, skillNames map[string][]str // Checks direct deps via manifest URLs and transitive deps via pinfile ref_type. func (r *Runner) checkNonTaggedDeps(result *Result, m *manifest.Manifest, p *pinfile.Pinfile) { // Check direct dependencies from manifest - for alias, url := range m.Dependencies { - parsed, err := resolve.ParseDepURL(url) + for alias, dep := range m.Dependencies { + parsed, err := resolve.ParseDepURL(dep.URL) if err != nil { continue } switch parsed.RefType { case resolve.RefTypeBranch: result.Warnings = append(result.Warnings, &Warning{ - Message: fmt.Sprintf("dependency %q tracks a branch (%s) — weaker reproducibility guarantees than tagged versions", alias, url), + Message: fmt.Sprintf("dependency %q tracks a branch (%s) — weaker reproducibility guarantees than tagged versions", alias, dep.URL), }) case resolve.RefTypeCommit: result.Warnings = append(result.Warnings, &Warning{ - Message: fmt.Sprintf("dependency %q uses a commit pin (%s) — reproducible but frozen; no updates available", alias, url), + Message: fmt.Sprintf("dependency %q uses a commit pin (%s) — reproducible but frozen; no updates available", alias, dep.URL), }) } }