test: add tests for skill discovery, assignment, and materialization

[geoffjay]

Adds comprehensive test coverage for the skills system.

New tests:

• skills.rs (7 new): materialize_skills() — writes SKILL.md, creates dirs, skips existing files, reports not_found, handles empty list, verifies file count
• storage.rs (3 new): skills round-trip through SQLite (add with skills, defaults empty, multi-skill round-trip)
• apply.rs (4 new): AgentTemplate.skills parsing — list form, all string, omitted default, empty list

Coverage: 493 unit tests passing (up from 483).

Closes #1214

[geoffjay]

Review: test: add tests for skill discovery, assignment, and materialization

Stack Position

Branch issue-1214 is stacked on issue-1213 (PR #1230). Full chain: main ← epic-1209 (#1226) ← issue-1210 (#1228) ← issue-1211 (#1229) ← issue-1213 (#1230) ← issue-1214 (#1231). All parent PRs are in merge-queue.

---

No Blocking Issues

---

Non-Blocking: SkillsField Invalid-String Path Is Still Untested

The original #1228 review flagged this specific path as worth covering:

# skills: typo  →  SkillsField::All("typo")  →  caught at apply time

Because #[serde(untagged)] matches any string as SkillsField::All(...), the string "typo" deserializes silently and the validation only fires in apply_agent. A test that serializes this YAML, calls into the apply path, and asserts the error message ("use 'all' or provide a list") would close the loop. The four current tests cover the happy and default paths but not the error branch.

---

Non-Blocking: I/O Error Paths in materialize_skills Are Not Covered

The implementation has two error branches (lines 109–128) that route write failures into result.not_found:

if let Err(e) = tokio::fs::create_dir_all(&target_dir).await { ... result.not_found.push(...) }
if let Err(e) = tokio::fs::write(&target_file, ...).await { ... result.not_found.push(...) }

Neither branch is exercised. Testing these portably requires permission manipulation (chmod 0o000) which is Unix-only and fragile, so omitting them is reasonable. Worth noting that a skill found-but-unwritable currently ends up in not_found (the categorization issue flagged in #1229 review).

---

Non-Blocking: PR Description Test Count Is Off

The description says "+10 tests (493 up from 483)" but the diff adds 14 test functions (7 in skills.rs, 3 in storage.rs, 4 in apply.rs). Minor documentation inaccuracy — not a code issue.

---

What's Working Well

• test_materialize_empty_skill_list_is_noop: The assertion assert!(!tmp.path().join(".claude").exists()) is an important invariant — verifies no filesystem side-effects when there's nothing to materialize. ✓
• test_materialize_multiple_skills: Uses written.sort() before the assertion — makes the test order-independent regardless of HashMap iteration order. ✓
• test_materialize_written_count_matches_files: Cross-checks result.written.len() against actual files on disk. This catches any case where the struct and filesystem diverge. ✓
• test_materialize_does_not_overwrite_existing_file: Verifies content preservation ("# My local version" survives), not just the skipped bucket. ✓
• make_discovered_skill helper: Constructs a valid Skill with realistic content. Uses skill.content (which is what tokio::fs::write uses) so the fake source_path is irrelevant and correct. ✓
• All three MaterializeResult buckets covered: written, skipped, and not_found each have at least one focused test. ✓
• SkillsField deserialization: all four variants covered: list, all string, omitted (default), and empty list []. ✓
• Storage round-trip: empty skills, 2-skill list, and 3-skill list all verified — order preservation through JSON serialization confirmed. ✓

---

Summary

No blocking issues. Three non-blocking notes: the apply-time error path for invalid strings (deferred from #1228 review) is the most actionable gap — a single additional test would close it. I/O error branches are hard to test portably and their omission is acceptable. PR description test count is slightly off. The core materialization, storage, and YAML parsing paths are all well-covered. Adding to merge-queue — this completes the capstone test work for the #1208 skills epic.