feat(skill): worktree provisioning and CLAUDE.md skill injection

[nextlevelshit]

Summary

• Implement worktree provisioning of resolved skills into pipeline step workspaces
• Add ProvisionSkills() function in internal/skill/provision.go that copies SKILL.md files into step worktrees at .wave/skills/<name>/
• Inject skill content into assembled runtime CLAUDE.md via adapter's InjectSkillContent() method
• Refactor executor to use the new provisioning flow, replacing the previous inline skill handling
• Deduplicate skills and resolve conflicts using scope precedence (more specific scope wins)

Closes #386

Changes

• internal/skill/provision.go (new) — ProvisionSkills() copies resolved skill SKILL.md files into step worktrees, handles deduplication and missing skill errors
• internal/skill/provision_test.go (new) — comprehensive tests for provisioning: happy path, missing skills, deduplication, conflict resolution
• internal/adapter/claude.go — InjectSkillContent() method to append skill SKILL.md content into runtime CLAUDE.md
• internal/adapter/claude_test.go — tests for skill content injection into CLAUDE.md
• internal/adapter/adapter.go — updated AdapterRunConfig to carry resolved skill metadata
• internal/pipeline/executor.go — integrated skill provisioning into step execution flow, removed old inline skill handling
• internal/pipeline/executor_test.go — executor-level tests for skill provisioning integration
• specs/386-skill-worktree-provisioning/ — specification, plan, and task artifacts

Test Plan

• Unit tests for ProvisionSkills() covering: successful provisioning, missing skills, deduplication, scope precedence
• Unit tests for InjectSkillContent() verifying CLAUDE.md assembly includes skill content
• Executor integration tests verifying end-to-end skill provisioning during step execution
• All tests pass with go test ./... and go test -race ./...

[nextlevelshit]

Code Review (Wave Pipeline)

Verdict: REQUEST_CHANGES

This PR is a well-structured refactor that extracts skill provisioning into a dedicated function and adds skill metadata injection into runtime CLAUDE.md. The direction is sound and improves testability, separation of concerns, and error handling. However, there is one bug and one breaking change that should be addressed before merge.

---

Critical Issues (must fix)

1. NewChildExecutor does not propagate skillStore

internal/pipeline/executor.go:206-224

NewChildExecutor() copies most executor fields to the child but omits skillStore. Child pipelines spawned via matrix strategies or composition steps will silently skip all DirectoryStore skill provisioning — the if e.skillStore != nil guard passes without warning or error.

Fix: Add skillStore: e.skillStore to the NewChildExecutor() return struct.

2. Breaking behavioral change needs migration path

internal/pipeline/executor.go:1261-1263

Missing DirectoryStore skills changed from warnings (continue with degraded execution) to hard errors (abort step). Existing pipelines that tolerate missing skills will start failing with no deprecation signal.

Fix: At minimum, document this in release notes. Consider a transitional release that emits deprecation warnings before making it a hard error, or add a strict option to skill configuration.

---

Suggested Improvements

• Sanitize skill descriptions before CLAUDE.md injection (internal/adapter/claude.go:784-796): s.Description is length-capped but not sanitized for Markdown heading syntax (#, ---). A malicious skill package could inject content that appears as a top-level CLAUDE.md directive. Strip heading markers or restrict to single-line content. (Flagged by both security and quality review.)

• Add symlink check at read time in ProvisionFromStore (internal/skill/provision.go:56-68): The source path for resource files is not checked for symlinks at read time. A narrow TOCTOU window exists between discoverResources() and os.ReadFile(). Add an os.Lstat check before reading to match the pattern used in containedPath().

• Remove unused SourcePath from SkillInfo (internal/skill/provision.go:11-15): The field is populated but never consumed downstream. Remove per YAGNI to avoid accidental future exposure of host filesystem paths.

• Rename skill.SkillInfo to ProvisionedSkill or ProvisionResult to distinguish it from the two other SkillInfo types in cmd/wave/commands/list.go and internal/tui/skill_provider.go.

• Add test coverage for: child executor skill propagation (H-01), empty skill body, concurrent provisioning to same workspace, and skill description with Markdown injection payload.

---

Positive Observations

• Improved fail-safe behavior: Upgrading silent skill-missing warnings to hard errors (once the migration path is handled) prevents steps from running with incomplete configuration — a meaningful security improvement.
• Path traversal protection preserved and tested: ProvisionFromStore correctly implements containment checks, and TestProvisionFromStore_PathTraversal explicitly validates that ../../../etc/passwd is blocked.
• Clean separation of concerns: Moving provisioning into internal/skill/provision.go makes the security-critical path logic independently auditable and testable.
• Comprehensive test coverage: 5 unit tests for ProvisionFromStore, 4 for buildSkillSection, plus integration tests for CLAUDE.md assembly and executor wiring.
• Skill names are safe in CLAUDE.md: ValidateName() restricts to ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$, making name-based injection impossible.
• No critical security vulnerabilities: Overall risk is LOW-MEDIUM. The two medium-severity security findings (prompt injection surface, TOCTOU race) represent defense-in-depth opportunities rather than exploitable vulnerabilities given the current trust model.

---

Generated by Wave gh-pr-review pipeline

[nextlevelshit]

Code Review (Wave Pipeline)

Verdict: APPROVE

Well-structured extraction of inline skill provisioning into skill.ProvisionFromStore() with good test coverage, clean separation of concerns, and a security improvement (fail-closed on missing skills). No exploitable vulnerabilities found through the current DirectoryStore implementation.

---

Critical Issues

None. All findings are defense-in-depth recommendations or code quality improvements — nothing blocks merge.

---

Suggested Improvements

1. Document the behavioral breaking change (HIGH — from quality review)

internal/pipeline/executor.go:1262-1263 — The old code warned and continued when a skill couldn't be provisioned; the new code returns a hard error that fails the step. This is architecturally correct (fail-closed), but pipelines that previously tolerated unresolvable skill references will now break.

Action: Document this in release notes. Consider whether a migration path is needed.

2. Add ValidateName call in ProvisionFromStore (MEDIUM — from security review)

internal/skill/provision.go:35 — The name parameter is used in filepath.Join without calling ValidateName(). The DirectoryStore.Read() validates internally, but the Store interface doesn't require it. A name like ../../../etc could escape the skill directory.

if err := ValidateName(name); err != nil {
    return nil, fmt.Errorf("skill %q: %w", name, err)
}

3. Sanitize skill descriptions before CLAUDE.md injection (MEDIUM — flagged by both reviews)

internal/adapter/claude.go:793 — Skill descriptions are interpolated directly into the CLAUDE.md prompt without sanitization. Currently safe (descriptions come from trusted on-disk SKILL.md frontmatter), but becomes a prompt injection vector if third-party skill registries are ever added.

desc := strings.ReplaceAll(s.Description, "\n", " ")
if len(desc) > 200 {
    desc = desc[:200]
}

4. Resolve type duplication: skill.SkillInfo vs adapter.SkillRef (MEDIUM — from quality review)

internal/skill/provision.go:11-15 and internal/adapter/adapter.go:32-35 — These represent the same concept with manual conversion at executor.go:1265-1269. Consider having the adapter accept skill.SkillInfo directly or defining a shared type.

5. Remove or justify SkillInfo.SourcePath (MEDIUM — from quality review)

internal/skill/provision.go:74 — SourcePath is populated on every SkillInfo but never consumed downstream. Either remove it or document its intended use.

6. Add empty description test case (MINOR — from quality review)

internal/adapter/claude_test.go — No test covers SkillRef{Name: "x", Description: ""}, which would produce - **x**: (see ...) with a dangling colon. Add a subtest and consider a "(no description)" fallback.

7. Fix comment numbering in prepareWorkspace (MINOR — from quality review)

internal/adapter/claude.go:279,295,305 — The skill section insertion created duplicate // 3. labels. Renumber to reflect actual order.

---

Positive Observations

• Clean extraction — The refactoring moves ~80 lines of inline provisioning logic into a well-scoped function with a clear interface, improving testability and separation of concerns.
• Thorough test coverage — New tests cover success, missing skill, empty list, path traversal, and multiple skills. Integration tests verify ResolvedSkills propagation and CLAUDE.md section ordering.
• Security improvement — Switching from warn-and-continue to hard error is fail-closed behavior, which is the correct default for skill provisioning.
• Path traversal protection preserved — The destination containment check (filepath.Abs + separator-aware prefix) is correctly implemented and tested.
• No new dependencies — Pure refactoring with no additional imports or external dependencies.
• All tests pass — Verified across internal/adapter, internal/skill, and internal/pipeline packages with no regressions.

---

Generated by Wave gh-pr-review pipeline