feat(cli): add wave skills subcommand for lifecycle management

[nextlevelshit]

Summary

• Adds wave skills top-level command with 5 subcommands: list, install, remove, search, sync
• All subcommands support --format table|json output and use structured CLIError codes
• list shows installed skills from project (.wave/skills/) and user (~/.claude/skills/) directories with pipeline usage cross-referencing
• install uses the skill router to handle source prefixes (tessl:, github:, file:, etc.) with dependency error classification
• remove includes interactive confirmation prompt with --yes flag to skip
• search and sync delegate to the tessl CLI with graceful missing-binary errors
• 16 test cases covering empty state, valid listings, JSON output, discovery warnings, error codes, confirmation flow, and missing dependencies

Spec

specs/382-skills-cli/spec.md

Test Plan

• All 16 new tests pass with -race flag
• Full go test -race ./... passes across all packages (29 packages)
• go vet ./... clean
• gofmt applied to all changed files

Known Limitations

• search and sync require the tessl CLI binary — returns skill_dependency_missing error if not installed
• Registry search output parsing assumes space-separated format from tessl search

Closes #382

[nextlevelshit]

Code Review (Wave Pipeline)

Verdict: APPROVE

Solid implementation of the wave skills command tree (list, install, remove, search, sync) with 16 passing tests, clean race detection, and proper integration with the existing internal/skill security infrastructure. No critical security vulnerabilities found — injection, path traversal, and SSRF are all properly mitigated by existing defenses.

---

Critical Issues

None. No CRITICAL or HIGH severity security vulnerabilities were identified. The implementation correctly uses exec.CommandContext with argument slices (no shell interpolation), validates skill names via strict regex, enforces path containment with symlink rejection, and blocks non-HTTPS sources.

---

Recommended Fixes (before or shortly after merge)

1. Capture stderr from tessl subprocess failures
skills.go:381, 479 — exec.CommandContext(...).Output() discards stderr. When tessl fails, users only see "exit status 1" with no diagnostic information. Extract err.(*exec.ExitError).Stderr to surface the actual error message.

2. Add timeout to tessl subprocess execution
skills.go:381, 479 — The context passed to exec.CommandContext has no deadline. A hanging tessl process blocks the CLI indefinitely. Wrap with context.WithTimeout(ctx, 30*time.Second).

3. Use t.Setenv() instead of os.Setenv() in tests
skills_test.go:341-343, 358-360 — Tests modify the global PATH variable to simulate missing tessl. This is unsafe under parallel test execution. t.Setenv() (Go 1.17+) handles restoration automatically and fails fast if called from a parallel test.

4. JSON output should use cmd.OutOrStdout() instead of os.Stdout
skills.go:151, 249, 343, 392, 501 — All JSON output paths bypass Cobra's output writer. This makes JSON output untestable via cmd.SetOut() (requiring a fragile os.Pipe() workaround) and breaks output routing when Cobra redirects stdout. Note: this matches the existing pattern in list.go:1584, so fixing it project-wide may be a separate effort.

5. Check os.Pipe() error in test helper
skills_test.go:59, 332, 366 — r, w, _ := os.Pipe() silently discards the error. Should be r, w, err := os.Pipe(); if err != nil { t.Fatal(err) }.

---

Suggested Improvements

• Add unit tests for output parsers — parseTesslSearchOutput and parseTesslSyncOutput handle external CLI output but have no dedicated tests. Table-driven tests with edge cases (empty output, malformed fields, single-field lines) would improve confidence.
• Validate rating field in search parser — skills.go:400-420 assumes fields[1] is a rating without checking it matches a star/number pattern. A simple heuristic check would prevent silent mis-parsing if tessl output format changes.
• Resolve project root for skill store path — skills.go:90-100 uses relative .wave/skills path, which breaks if the command is run from a subdirectory.
• Sanitize ANSI escape sequences from tessl output — skills.go:351-369, 433-453 renders external command output directly to the terminal. Stripping terminal control sequences would harden the trust boundary (low risk given tessl is user-installed).
• Nil vs empty slice inconsistency — SkillListOutput.Skills serializes as [] but Warnings may serialize as null. Consider initializing both consistently.

---

Positive Observations

• Strong security posture: No shell interpolation anywhere — all subprocess calls use argument slices. Skill names validated via strict regex (^[a-z0-9]([a-z0-9-]*[a-z0-9])?$). Path traversal protection with symlink detection. HTTPS-only enforcement for remote sources.
• Well-structured error handling: All errors wrapped in CLIError with appropriate codes and user-facing messages. Three new error codes (CodeSkillNotFound, CodeSkillSourceError, CodeSkillValidationError) follow existing conventions.
• Good test coverage: 16 tests covering all subcommands including edge cases (interactive confirmation y/n, --yes flag, JSON output format, discovery warnings, missing dependencies).
• Consistent codebase integration: Follows established patterns — Cobra command tree, ResolveFormat, display.NewFormatter, CLIError classification. Clean integration with internal/skill package.
• UX polish: Confirmation prompt for destructive remove action, empty state hints with install guidance, warning display for skill discovery issues.

---

Generated by Wave gh-pr-review pipeline