feat: Subpath Skill Selection — cherry-pick skills from large packages

[erdemtuna]

Summary

Add subpath-based skill selection so consumers can install specific skills from large packages instead of getting everything.

What Changed

New: Structured dependency format in craft.yaml

dependencies:
  # Install all skills (existing behavior, unchanged)
  tools: github.com/example/tools@v1.0.0

  # Install only selected skills
  acme:
    url: github.com/acme/skills@v1.0.0
    select:
      - skills/docx
      - skills/pdf

New: craft get with #subpath

craft get github.com/acme/skills@v1.0.0#skills/docx

New: Interactive skill selection in craft add

When adding a multi-skill package, craft add now shows available skills and lets you pick which ones to install. Use --all to skip the prompt.

New: craft update informs about new skills

When updating a selectively installed package, craft tells you about newly available skills.

Implementation (7 phases)

1. Core Type System — DependencySpec type replacing map[string]string (19 files)
2. DepURL Subpath Parsing — #subpath fragment support with PURL-compatible syntax
3. Resolver Filtering — Filter skills by select, force auto-discovery for FR-006
4. CLI craft get — Parse #subpath from URL argument
5. Interactive craft add — MultiSelect UI, --all flag, TTY detection
6. craft update Discovery — Inform about newly available skills
7. Documentation — README and technical docs

Key Design Decisions

• PURL-compatible syntax: Uses # fragment separator aligned with ECMA-427
• Consumer selection overrides exports: select scans all discoverable skills, not just package exports
• Format canonicalization: Structured deps with empty select normalize to simple strings
• Backward compatible: All existing craft.yaml files work unchanged

Testing

• All 15 test packages pass
• New tests: structured dep round-trip, select path validation, resolver filtering, MVS merge, export override
• Linter and vet clean

References

• Originated from: agentskills/agentskills#210
• PURL spec: https://github.com/package-url/purl-spec

[erdemtuna]

Must-address · Correctness

When a user runs craft add github.com/acme/skills@v1.0.0#skills/docx in a TTY, the #skills/docx subpath is set at line 110, then immediately overwritten by the interactive prompt result at line 122. The user's explicit subpath selection is silently replaced by whatever they pick interactively, and the #subpath in the URL becomes misleading.

Additionally, the error vs. success paths produce inconsistent behavior: if discoverAndSelect returns an error (line 119-120), the warning is logged but the original subpath-based Select is preserved. If it succeeds, the subpath Select is overwritten. This means errors silently produce different installed skill sets than success.

Suggested change

	// Interactive skill selection: if running in a TTY with multiple
	// skills available, let the user pick which skills to include.
	if isInteractive {
	selected, err := discoverAndSelect(cmd, parsed)
	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}
	}
	}
	// Interactive skill selection: if running in a TTY with multiple
	// skills available, let the user pick which skills to include.
	// When the user explicitly provided a #subpath, honor that intent
	// and skip the interactive prompt.
	if isInteractive && parsed.Subpath == "" {
	selected, err := discoverAndSelect(cmd, parsed)
	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}
	}
	}

[erdemtuna]

Must-address · Correctness

The two normalization functions disagree on whitespace handling. normalizeSubpath (depurl.go:196) includes strings.TrimSpace(), but normalizeSelectPaths here does not. A YAML select entry with leading/trailing whitespace (e.g., " skills/docx ") passes through normalizeSelectPaths unchanged, producing a path that will never match in filterBySelect's dirIndex lookup (resolver.go:637), resulting in a confusing "does not match any skill" error.

Suggested change

	for i, p := range paths {
	p = strings.TrimPrefix(p, "./")
	p = strings.TrimSuffix(p, "/")
	out[i] = p
	for i, p := range paths {
	p = strings.TrimSpace(p) // Match normalizeSubpath behavior
	p = strings.TrimPrefix(p, "./")
	p = strings.TrimSuffix(p, "/")
	out[i] = p

Alternatively, extract a shared normalizePath function used by both call sites to guarantee consistency:

// Shared normalization for subpath/select paths.
func normalizePath(s string) string {
    s = strings.TrimSpace(s)
    s = strings.TrimPrefix(s, "./")
    s = strings.TrimSuffix(s, "/")
    return s
}

[erdemtuna]

Should-address · Maintainability / Performance

discoverAndSelect creates its own newFetcher() at line 220, separate from the fetcher created at line 127 for resolve validation. Since the ListTree cache is per-fetcher-instance, the skill discovery in discoverAndSelect cannot share cached tree data with the subsequent resolve step. Both calls independently clone/fetch the same repository.

Other CLI commands (update.go:83, get.go) create a single fetcher instance and reuse it. Consider passing the fetcher as a parameter:

Suggested change

	func discoverAndSelect(cmd *cobra.Command, parsed *resolve.DepURL) ([]string, error) {
	fetcher, err := newFetcher()
	func discoverAndSelect(cmd *cobra.Command, parsed *resolve.DepURL, fetcher fetch.Fetcher) ([]string, error) {
	// Remove: fetcher, err := newFetcher()

Then at the call site, create the fetcher earlier and pass it through.

[erdemtuna]

Should-address · Correctness / Testing

The prefix-based file matching correctly appends / to the select path (prefix := sel + "/"), which prevents prefix collisions (e.g., selecting skills/doc won't match files under skills/docx/). However, there's no test verifying this specific scenario, and the trailing-slash defense is subtle enough that a future refactor could break it without a regression test.

Add a test case in resolver_test.go with overlapping skill prefixes:

func TestFilterBySelectPrefixCollision(t *testing.T) {
    names := []string{"doc", "docx"}
    dirs := []string{"skills/doc", "skills/docx"}
    files := map[string][]byte{
        "skills/doc/craft-skill.yaml":  []byte("name: doc"),
        "skills/doc/README.md":         []byte("doc readme"),
        "skills/docx/craft-skill.yaml": []byte("name: docx"),
        "skills/docx/README.md":        []byte("docx readme"),
    }

    gotNames, gotDirs, gotFiles, err := filterBySelect(names, dirs, files, []string{"skills/doc"})
    if err != nil {
        t.Fatalf("unexpected error: %v", err)
    }
    // Should include only "doc", not "docx"
    if len(gotNames) != 1 || gotNames[0] != "doc" {
        t.Errorf("expected [doc], got %v", gotNames)
    }
    if len(gotDirs) != 1 || gotDirs[0] != "skills/doc" {
        t.Errorf("expected [skills/doc], got %v", gotDirs)
    }
    if len(gotFiles) != 2 {
        t.Errorf("expected 2 files, got %d", len(gotFiles))
    }
}

[erdemtuna]

Should-address · Testing

Select preservation on version bump is a key user expectation — when craft update bumps a package version, the user's skill selection should be carried forward. This line correctly preserves depSpec.Select, but there's no test verifying this behavior. If a future refactor accidentally drops Select from the DependencySpec construction, users would silently lose their skill selection after craft update.

Add a test that creates a manifest with Select, simulates a version bump, and verifies Select is preserved:

func TestUpdatePreservesSelect(t *testing.T) {
    m := &manifest.Manifest{
        SchemaVersion: 1,
        Name:         "test",
        Skills:       []string{"skills/test"},
        Dependencies: map[string]manifest.DependencySpec{
            "tools": {URL: "github.com/acme/tools@v1.0.0", Select: []string{"skills/docx"}},
        },
    }

    depSpec := m.Dependencies["tools"]
    parsed, _ := resolve.ParseDepURL(depSpec.URL)
    newURL := parsed.WithVersion("v2.0.0")
    m.Dependencies["tools"] = manifest.DependencySpec{URL: newURL, Select: depSpec.Select}

    updated := m.Dependencies["tools"]
    if len(updated.Select) != 1 || updated.Select[0] != "skills/docx" {
        t.Errorf("expected Select [skills/docx], got %v", updated.Select)
    }
}

[erdemtuna]

Should-address · Testing

The UnmarshalYAML method handles three node kinds (ScalarNode, MappingNode, default error). Only TestValidateStructuredDepMissingURL (validate_test.go:306-330) tests the missing-URL case via a full YAML parse. No direct unit tests exist for: sequence node input, null node, or empty scalar string.

Add direct unit tests for each code path:

func TestDependencySpecUnmarshalYAML(t *testing.T) {
    tests := []struct {
        name    string
        yaml    string
        wantURL string
        wantSel []string
        wantErr bool
    }{
        {"scalar string", `github.com/acme/tools@v1.0.0`, "github.com/acme/tools@v1.0.0", nil, false},
        {"mapping with select", "url: github.com/acme/tools@v1.0.0\nselect:\n  - skills/docx", "github.com/acme/tools@v1.0.0", []string{"skills/docx"}, false},
        {"mapping url only", "url: github.com/acme/tools@v1.0.0", "github.com/acme/tools@v1.0.0", nil, false},
        {"mapping no url", "select:\n  - skills/docx", "", nil, true},
        {"sequence node", "[a, b]", "", nil, true},
    }
    // ... run sub-tests
}

Minor note: The error message at line 45 formats the Kind as an integer (got %v → e.g., got 4) rather than a human-readable type name. Consider using "got YAML node kind %d" for slightly clearer context, though this is a low-priority improvement since yaml.v3 doesn't provide a Kind.String() method and the error path is unlikely to trigger in practice.

[erdemtuna]

Should-address · Maintainability

The new-skill notification uses filepath.Base(p) to extract display names from skill paths. If two new skills share the same base name but different paths (e.g., skills/v1/auth and skills/v2/auth), the notification would show: "New skills available: auth, auth" — duplicate entries with no distinguishing context.

Use the full relative path for clarity:

Suggested change

	newSkills = append(newSkills, filepath.Base(p))
	newSkills = append(newSkills, p)

[erdemtuna]

Could-consider · Maintainability

The DepURL.Raw field is populated here but String() (lines 185-191) was changed to always reconstruct from components instead of returning Raw. No production code reads Raw, though it is verified in tests. It's effectively vestigial state — set but not consumed in production.

Consider adding a brief comment on the field declaration (line 42) documenting its purpose, e.g.:

// Raw preserves the original (fragment-stripped) URL for diagnostic use and test verification.
Raw string

This clarifies intent for future maintainers who may otherwise remove it as dead code.

[erdemtuna]

Could-consider · Maintainability

If MultiSelect is called with items = []string{}, it prints the prompt and numbered list (no items), then asks for input. The user sees "Enter numbers to include..." but there's nothing to select. The current call site (discoverAndSelect at add.go:259) guards with len(skills) <= 1, but MultiSelect is a reusable UI component that should handle degenerate inputs gracefully.

Suggested change

	func MultiSelect(prompt string, items []string, w io.Writer, r io.Reader) ([]int, error) {
	_, _ = 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: ")
	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: ")

[erdemtuna]

Could-consider · Maintainability

The empty-sel guard handles the case where a select path normalizes to empty string, causing all files to be included. Given that validation at validate.go:61-62 rejects empty select paths before they reach the resolver, this guard is effectively defense-in-depth. Adding a brief comment would prevent future maintainers from removing it as "unnecessary."

Suggested change

	if sel == "" {
	prefix = ""
	}
	// Safety: empty sel should be rejected by manifest validation;
	// guard against inclusion of all files if validation is bypassed.
	if sel == "" {
	prefix = ""
	}