feat(orchestrator): add Skill model and discovery from .agentd/skills/

[geoffjay]

Implements the foundation of the #1208 skills epic: skill model, frontmatter parsing, filesystem discovery, and the GET /skills API endpoint.

What was added

crates/orchestrator/src/skills.rs (new module):

• Skill struct — name, description, content, source_path
• parse_frontmatter() — zero-dependency ---…--- block parser; extracts name and description fields
• discover_skills(dir) — scans a directory for both layouts: <name>/SKILL.md and <name>.md; results sorted by name
• discover_all_skills() — merges .agentd/skills/ (project-level) and ~/.config/agentd/skills/ (user-level) with project taking precedence on name collision

crates/orchestrator/src/api.rs:

• GET /skills route added — returns discover_all_skills() as a JSON array; no auth required (read-only, local filesystem)

crates/orchestrator/src/lib.rs:

• pub mod skills; declaration added

Tests (16, all passing)

Frontmatter: name+description, name-only, quoted values, no frontmatter, unclosed block, empty block.
Discovery: missing dir, empty dir, directory layout, flat layout, fallback names, non-.md ignored, mixed layouts sorted, malformed file skipped.

Closes #1209

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.77%. Comparing base (31940cb) to head (78cee53).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1226   +/-   ##
=======================================
  Coverage   63.77%   63.77%           
=======================================
  Files         173      173           
  Lines        7733     7733           
  Branches     2566     2614   +48     
=======================================
  Hits         4932     4932           
  Misses       2780     2780           
  Partials       21       21           

Flag	Coverage Δ
frontend	63.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[geoffjay]

Review: feat(orchestrator): add Skill model and discovery from .agentd/skills/

Stack Position

Branch issue-1209 is based on main — no parent PR. This is the root of the #1208 skills epic; #1210, #1211, and #1213 all stack on this and cannot proceed until it lands.

---

Blocking: discover_all_skills() Is Not Globally Sorted

The GET /skills handler doc promises: "The list is sorted by name."

discover_skills() sorts results within each directory, but discover_all_skills() concatenates those sorted subsets without a final sort:

// Current — result is sorted per-directory, not globally:
// project skills: ["beta", "zap"]
// user skills:    ["alpha", "gamma"]
// result:         ["beta", "zap", "alpha", "gamma"]  ← wrong

pub fn discover_all_skills() -> Vec<Skill> {
    ...
    for dir in skill_search_dirs() {
        if let Ok(skills) = discover_skills(&dir) {
            for skill in skills {
                if seen.insert(skill.name.clone()) {
                    result.push(skill);  // no global sort
                }
            }
        }
    }
    result  // ← missing sort
}

Fix: add one line before result:

    result.sort_by(|a, b| a.name.cmp(&b.name));
    result

This is a correctness bug relative to the documented API contract. The single-directory case works fine, but the multi-directory case (project + user) produces non-deterministic ordering depending on which names appear in which source.

---

Non-Blocking: Per-Entry read_dir Errors Abort the Directory Scan

The doc for discover_skills says: "Files that cannot be read or are otherwise invalid are silently skipped."

That holds for load_skill_file errors (if let Ok(skill) = ...), but individual DirEntry failures propagate and abort the entire scan:

for entry in std::fs::read_dir(skills_dir)? {
    let entry = entry?;   // ← aborts the loop on any entry error

A single permission-denied entry mid-scan would cause discover_skills to return Err, which discover_all_skills then silently drops — so the whole directory is skipped rather than just the bad entry. Consistent fix:

    let Ok(entry) = entry else { continue; };

In practice this is extremely rare (directory entry reads rarely fail after read_dir succeeds), but it aligns the implementation with the stated contract.

---

Non-Blocking: source_path Exposes Server Filesystem Paths via API

Skill.source_path serializes to an absolute path string in the GET /skills JSON response (e.g. "/home/user/project/.agentd/skills/git-spice/SKILL.md"). API consumers — including the CLI in #1213 — don't need to know where on the server's filesystem a skill lives. Consider:

// Option A — skip in serialization (simplest):
#[serde(skip)]
pub source_path: PathBuf,

// Option B — keep source_path for internal use, add a separate API response type

source_path is genuinely useful internally (for #1211 materialization — knowing which file to copy). Keeping it pub(crate) with #[serde(skip)] preserves that without leaking it.

---

Non-Blocking: Relative CWD Path for Project Skills

let mut dirs = vec![PathBuf::from(".agentd/skills")];

This resolves relative to the orchestrator's current working directory at runtime. In development (started from the repo root) this works perfectly. In production (systemd service, Docker, etc.) the CWD is often /, /var/lib/..., or wherever the init system drops the process — meaning project skills would silently not be discovered.

A comment documenting this assumption (or a future config key for the project root) would prevent a hard-to-debug production surprise.

---

Non-Blocking: No Intra-Directory Deduplication

If both foo/SKILL.md and foo.md exist in the same directory and their frontmatter both resolve to name: foo, discover_skills will return two Skill structs with the same name. The dedup in discover_all_skills only operates across source directories, not within one. Rare edge case, but worth a note or an is_none() check after the find("\n---").

---

What's Working Well

• Frontmatter parser is minimal and correct for the expected input. strip_prefix("---") + find("\n---") correctly handles the open/close delimiters; v.trim().trim_matches('"') correctly strips both quoted and unquoted values including CRLF (trim() handles \r).
• discover_all_skills precedence model is right: HashSet<String> insertion-order dedup ensures project-level wins, and per-directory errors are swallowed so a missing user dir never blocks project skills.
• Test coverage is solid: 16 tests exercise all documented paths — both layouts, fallback names, non-.md filtering, empty/missing dirs, mixed-and-sorted output.
• tempfile correctly in [dev-dependencies] — not shipped in production binary.
• GET /skills returns [] (not 500) when no skills exist — consistent with discover_all_skills never erroring.
• Module is pub mod skills — correctly exposed for downstream crates (#1210, #1211) to import discover_skills / Skill.

---

Summary

One blocking issue: discover_all_skills() doesn't do a final sort, violating the GET /skills "sorted by name" guarantee when both project and user skill directories are populated. Fix is a single result.sort_by(...) call. The remaining notes are non-blocking. Once fixed this is ready to land and unblock the three downstream issues.

[geoffjay]

Re-Review: feat(orchestrator): add Skill model and discovery from .agentd/skills/

Blocking Issues — Both Resolved ✓

1. Global sort in discover_all_skills() — Fixed.

// Sort globally so the merged result is deterministic regardless of which
// names came from the project-level vs. user-level directory.
result.sort_by(|a, b| a.name.cmp(&b.name));
result

The explanatory comment is a nice addition. The new test test_discover_all_skills_global_sort_across_directories uses exactly the scenario from the original review (project: ["beta", "zap"], user: ["alpha", "gamma"] → merged: ["alpha", "beta", "gamma", "zap"]) — this would have caught the original bug.

2. Per-entry read_dir error handling — Fixed.

let Ok(entry) = entry else { continue };

Matches the exact fix requested, with a comment explaining intent. ✓

---

Non-Blocking Items — Also Addressed

source_path API leak — Fixed proactively. source_path is now pub(crate) with #[serde(skip)], and the new test_source_path_not_included_in_json test verifies it doesn't appear in serialized output. ✓

CWD assumption in production — Documented. The comment in skill_search_dirs() accurately describes the production CWD concern and names AGENTD_PROJECT_ROOT as a future config key. ✓

---

Summary

All issues from the initial review are resolved. The code is correct, the test suite is solid (16 tests covering all documented paths), and the fixes are clean. Adding to merge queue — this unblocks #1228 and #1229.