From 8c7fc35a9d13cab743f95d8a542161b73b0a64e1 Mon Sep 17 00:00:00 2001 From: Scott Stauffer Date: Fri, 28 Aug 2026 21:49:53 -0500 Subject: [PATCH 1/2] feat(skills): bring skill implementation to agentskills.io conformance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The REPL loader never read or validated the `name:` frontmatter field (it used the directory name unconditionally) and ignored license/compatibility/ metadata/allowed-tools entirely, while orchestration's Microsoft.Agents.AI loader already enforced the full spec — so a skill could work in one surface and silently vanish from the other with no diagnostic. Two related bugs compounded this: `skills add` and skill curation could install a SKILL.md whose `name:` field didn't match the directory it was written under, and read_skill_resource/run_skill_script only did a lexical path-containment check, so a symlink planted inside a skill directory could escape it. Introduces src/Core/Skills/SkillFrontmatter.cs (parsing + validation mirroring Microsoft's AgentSkillFrontmatter rules exactly) and SkillPathGuard.cs (symlink-safe path resolution) as the single source of truth all three skill-authoring surfaces now share, instead of three separate ad hoc regexes. The REPL loader keeps its lenient fallback for skills with no frontmatter, but now validates any name/description/ compatibility field that is declared and skips (with a warning) one that violates the spec; its discovery walk is now bounded and symlink-safe to match orchestration. `skills add` and curation canonicalize the `name:` field to the installed slug before writing. Adds `fuseraft skills validate` (fuseraft's equivalent of the spec's own skills-ref validate tool) and Requires/Spec columns on `skills list`. --- docs/cli-reference.md | 32 +- docs/security.md | 1 + docs/skills.md | 19 +- skills/skill-author/SKILL.md | 14 +- src/Cli/Commands/Repl/ReplCommand.cs | 6 +- src/Cli/Commands/Repl/ReplSkillsLoader.cs | 212 +++++++++++--- src/Cli/Commands/Skills/SkillsAddCommand.cs | 27 +- src/Cli/Commands/Skills/SkillsHelpers.cs | 38 +-- src/Cli/Commands/Skills/SkillsListCommand.cs | 30 +- .../Commands/Skills/SkillsValidateCommand.cs | 88 ++++++ src/Core/Skills/SkillFrontmatter.cs | 266 +++++++++++++++++ src/Core/Skills/SkillPathGuard.cs | 71 +++++ src/Infrastructure/Plugins/SkillsPlugin.cs | 22 +- src/Orchestration/Skills/SkillCurator.cs | 49 +++- src/Program.cs | 6 + .../ReplSkillsLoaderTests.cs | 131 +++++++++ .../SkillFrontmatterSpecTests.cs | 274 ++++++++++++++++++ .../FuseraftCli.Tests/SkillPathGuardTests.cs | 155 ++++++++++ tests/FuseraftCli.Tests/SkillsHelpersTests.cs | 33 +++ tests/FuseraftCli.Tests/SkillsPluginTests.cs | 50 ++++ 20 files changed, 1425 insertions(+), 99 deletions(-) create mode 100644 src/Cli/Commands/Skills/SkillsValidateCommand.cs create mode 100644 src/Core/Skills/SkillFrontmatter.cs create mode 100644 src/Core/Skills/SkillPathGuard.cs create mode 100644 tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs create mode 100644 tests/FuseraftCli.Tests/SkillPathGuardTests.cs diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 0e3075f8..b8b1db51 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -1965,7 +1965,7 @@ Jobs can be edited by hand — `fuseraft schedule run` reads the YAML fresh on e ## `fuseraft skills` -Install, list, and remove global skills available to all agent sessions. Skills are stored in `~/.fuseraft/skills/` and registered in an FTS5 search index so fuseraft can automatically identify which ones are relevant to a given task. +Install, list, remove, and validate global skills available to all agent sessions. Skills are stored in `~/.fuseraft/skills/` and registered in an FTS5 search index so fuseraft can automatically identify which ones are relevant to a given task. See [Skills](skills.md) for an overview of how skills work and how to write them. @@ -2008,7 +2008,7 @@ List all installed global skills. fuseraft skills list ``` -Displays a table with the slug and description for each skill found under `~/.fuseraft/skills/`. +Displays a table with the slug, description, `compatibility` field (if any), and Agent Skills specification conformance (`✓`/`✗`) for each skill found under `~/.fuseraft/skills/`. Run `fuseraft skills validate` for details on any `✗` entries. **Examples** @@ -2074,6 +2074,34 @@ See [Configuration → Skill curation](configuration.md#skill-curation) for the --- +### `fuseraft skills validate` + +Validate a `SKILL.md`'s frontmatter against the [Agent Skills specification](https://agentskills.io/specification) — fuseraft's equivalent of the spec's own recommended `skills-ref validate` tool. Checks the `name` field's format, length, and match against its parent directory name; the `description` field's presence and length; and the `compatibility` field's length. Uses the same validator fuseraft's orchestration skills provider applies at load time, so a skill that passes here is guaranteed to load identically in both the REPL and `fuseraft run` sessions. + +``` +fuseraft skills validate [path] +``` + +**Arguments** + +| Argument | Description | +|----------|-------------| +| `[path]` | Path to a skill directory to validate. Omitted: validates every skill under `~/.fuseraft/skills/`. | + +Exits with status `0` when every checked skill is fully conformant, `1` otherwise. + +**Examples** + +```bash +# Validate every installed skill +fuseraft skills validate + +# Validate a skill before installing it +fuseraft skills validate ../skills/sandbox-test +``` + +--- + ## `fuseraft log` View fuseraft log files. Orchestration session logs (`fuseraft log events`) are read from the global `~/.fuseraft/logs/sessions/` directory. REPL and application logs are read from the current project's `.fuseraft/logs/` directory. diff --git a/docs/security.md b/docs/security.md index feb9eddb..0ddcf6df 100644 --- a/docs/security.md +++ b/docs/security.md @@ -435,6 +435,7 @@ If `fuseraft run --work-dir` points at a directory you did not author, any skill - Only run `fuseraft` in working directories you trust. Treat `.agents/skills/` and `.fuseraft/skills/` in a cloned repo the same way you would treat a `Makefile` or `package.json` postinstall script. - For higher assurance, run fuseraft inside a Docker container (`CodeExecution` plugin) where the host environment is not exposed. - `UseScriptApproval` support is planned — when enabled it will require explicit user confirmation before any skill script executes. Until then, script execution is automatic once a skill is loaded. +- `read_skill_resource` and `run_skill_script` resolve the model-supplied path against the skill directory and reject anything that resolves outside it, including via a symlinked file or subdirectory planted inside the skill folder — this narrows path-based escape from *within* a loaded skill, but a fully malicious skill script still runs as an OS subprocess with the full process environment; it isn't a substitute for only loading trusted skills. --- diff --git a/docs/skills.md b/docs/skills.md index 644720bc..7cd64db8 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -36,6 +36,8 @@ At startup, the skill count appears in the compact info line alongside the activ | `read_skill_resource` | Read a supplementary file bundled with a skill (e.g. a file under `references/`), by path relative to the skill directory. | | `run_skill_script` | Run a script bundled with a skill (`.sh`, `.py`, `.js`). | +`read_skill_resource` and `run_skill_script` reject a path that resolves outside the skill directory, including via a symlinked file or subdirectory planted inside it. + If `--no-tools` is passed, skills are disabled for that session. `fuseraft run` orchestration sessions use the same five discovery locations and the same three tools (`load_skill`, `read_skill_resource`, `run_skill_script`), wired onto every agent automatically whenever at least one skill directory exists — there is no need to add `Skills` to an agent's `Plugins:` list, though doing so as a declaration of intent is harmless. @@ -182,6 +184,8 @@ The command accepts a path to a skill directory (containing `SKILL.md`) or direc You can also install skills by placing them directly under `~/.fuseraft/skills/` without using the CLI — skills are loaded from that directory at session start regardless of how they got there. +`fuseraft skills add` canonicalizes the frontmatter as it installs: if the raw `name:` field doesn't already equal the slug it's being installed under (e.g. it had spaces or uppercase letters), the installed copy's `name:` line is rewritten to match. This guarantees an installed skill's `name:` and directory always agree, which orchestration requires (see below). + --- ## Writing a skill @@ -203,13 +207,22 @@ description: What this skill does and when to use it. Step-by-step guidance for the agent... ``` -The `name` field is used by `fuseraft skills add` to derive the destination directory name when installing a skill globally, so keeping it in sync with the directory name is strongly recommended. The `description` is what fuseraft uses to decide whether the skill is relevant to the current task — write it so it covers both what the skill does and the kinds of tasks that should trigger it. +fuseraft follows the [Agent Skills specification](https://agentskills.io/specification) for `SKILL.md` frontmatter: + +| Field | Required | Notes | +|-------|----------|-------| +| `name` | Yes | Lowercase letters, digits, and single hyphens only (no leading/trailing/double hyphens); max 64 characters; must match the parent directory name exactly. | +| `description` | Yes | 1–1024 characters. What fuseraft uses to decide whether the skill is relevant to the current task — write it so it covers both what the skill does and the kinds of tasks that should trigger it. | +| `license` | No | License name, or a reference to a bundled license file. | +| `compatibility` | No | Max 500 characters. Environment requirements (e.g. `Requires docker and jq`) — shown in the REPL's skill catalog as a `[requires: ...]` hint. | +| `metadata` | No | Arbitrary string-to-string map for your own bookkeeping (author, version, etc.). Not surfaced to the model. | +| `allowed-tools` | No | Space-separated list of pre-approved tools (experimental, per spec — fuseraft parses but does not currently act on this field). | -If your instructions are long, move reference material into a `references/` subdirectory inside the skill folder. The agent loads those files on demand — with `read_skill_resource` — rather than all at once. +If your instructions are long, move reference material into a `references/` subdirectory inside the skill folder. The agent loads those files on demand — with `read_skill_resource` — rather than all at once. `scripts/` and `assets/` are supported the same way. **If two installed skills share the same name**, the one in the higher-precedence location wins and a warning is logged. -> **Keep `name:` and the directory name identical.** The REPL loader uses the directory name as the slug and never reads `name:` at load time, so a mismatch is harmless there. `fuseraft run` orchestration sessions use a stricter loader that requires `name:` to match the directory name **exactly** (case-sensitive), to be non-empty lowercase kebab-case (letters, digits, single hyphens — no leading/trailing/double hyphens), and requires a non-empty `description:`. A skill that violates any of these is silently dropped from the orchestration catalog — it works fine in the REPL but an agent in a `fuseraft run` session never sees it. Follow the frontmatter format above exactly and both surfaces will pick up the skill identically. +> **Keep `name:` and the directory name identical.** `fuseraft run` orchestration sessions require `name:` to match the directory name **exactly** (case-sensitive), to be valid lowercase kebab-case, and require a non-empty, correctly-sized `description:`. A skill that violates any of these is silently dropped from the orchestration catalog. The REPL's loader is more lenient — a `SKILL.md` with no frontmatter at all still loads, using the directory name as its slug — but the moment a `name:`, `description:`, or `compatibility:` field *is* declared, the REPL validates it against the same rules and **skips the skill with a warning** on a violation (most commonly a name/directory mismatch), rather than silently loading something that would vanish under `fuseraft run`. Run `fuseraft skills validate [path]` to check a skill (or every installed skill) against the full specification before relying on it. --- diff --git a/skills/skill-author/SKILL.md b/skills/skill-author/SKILL.md index 919e498d..87974517 100644 --- a/skills/skill-author/SKILL.md +++ b/skills/skill-author/SKILL.md @@ -44,7 +44,13 @@ description: --- ``` -**`name`:** A short, lowercase kebab-case slug (e.g. `debug-session`, `mcp-setup`) — letters, digits, and single hyphens only, no leading/trailing/double hyphens. This is used as the install directory name when running `fuseraft skills add`. Keep it to 1–3 words, and **make it identical to the skill's directory name**: the REPL loader ignores `name:` and uses the directory name as the slug, but `fuseraft run` orchestration sessions use a stricter loader that silently drops the skill from the catalog if `name:` doesn't exactly match the directory name (or isn't valid kebab-case, or `description:` is empty). Matching them keeps the skill working identically in both surfaces. +**`name`:** A short, lowercase kebab-case slug (e.g. `debug-session`, `mcp-setup`) — letters, digits, and single hyphens only, no leading/trailing/double hyphens, max 64 characters. This is used as the install directory name when running `fuseraft skills add`. Keep it to 1–3 words, and **make it identical to the skill's directory name**: `fuseraft run` orchestration sessions silently drop the skill from the catalog if `name:` doesn't exactly match the directory name (or isn't valid kebab-case, or `description:` is empty or too long). The REPL loader is more lenient about a skill with no frontmatter at all, but once `name:` is present it applies the same check and skips the skill (with a warning) on a mismatch. Matching them keeps the skill working identically in both surfaces — run `fuseraft skills validate ` to confirm before installing. + +**Optional fields**, per the [Agent Skills specification](https://agentskills.io/specification) — add only when they earn their keep: +- **`license`:** a license name or reference to a bundled license file. Only relevant for skills you intend to share/distribute. +- **`compatibility`:** environment requirements, max 500 characters (e.g. `Requires docker and jq`, `Designed for fuseraft REPL sessions`). Shown to the agent in the REPL catalog as a `[requires: ...]` hint — add it when the skill assumes a tool or platform that isn't universally available. +- **`metadata`:** a string-to-string map for your own bookkeeping (e.g. `author`, `version`). Not shown to the agent. +- **`allowed-tools`:** experimental per spec; fuseraft parses it but doesn't currently act on it. Skip it. **`description`:** This is the most important field — fuseraft injects only the name and description into the agent's catalog at session start. The agent reads this to decide whether the skill is relevant. Write it so it covers: - What the skill produces or accomplishes @@ -168,7 +174,9 @@ Or write directly to `~/.fuseraft/skills//SKILL.md` — fuseraft loads fro ### Step 7: Verify -For **REPL sessions**, start or restart fuseraft and run `/tools`. The skill should appear under the `Skills` category with its name and description. +First, run `fuseraft skills validate ` (or `fuseraft skills validate` with no argument once installed, to check it alongside every other installed skill). This checks the frontmatter against the full specification — name format and directory match, description presence/length, compatibility length — with the same validator both the REPL and orchestration use, before you burn a session on it. + +For **REPL sessions**, start or restart fuseraft and run `/tools`. The skill should appear under the `Skills` category with its name and description. Watch the startup output for a `⚠ Skipped skill at ...` warning — that means the frontmatter is present but invalid, and the skill did not load. For **orchestration sessions**, run `fuseraft validate` on the config first, then do a one-turn dry run: @@ -179,7 +187,7 @@ fuseraft run --config --max-iterations 1 "List your available skills." The agent should name the skill in its response. If it does not appear, check: - `SKILL.md` is directly inside the skill directory (not nested deeper) - The install path is one of the five recognized locations (project `.fuseraft/skills/`, project `.agents/skills/`, user `.fuseraft/skills/`, user `.agents/skills/`, or shipped built-in) -- **Orchestration-only:** `name:` in the frontmatter exactly matches the directory name (case-sensitive), is valid lowercase kebab-case, and `description:` is non-empty — a mismatch here loads fine in the REPL but is silently dropped by `fuseraft run`'s stricter loader with no error to the user, only a log entry +- `fuseraft skills validate` passes — a violation it reports is silently dropped by `fuseraft run`'s stricter loader with no error to the user, only a log entry ### Step 8: Refine the Description diff --git a/src/Cli/Commands/Repl/ReplCommand.cs b/src/Cli/Commands/Repl/ReplCommand.cs index e2e12efe..b5b2af14 100644 --- a/src/Cli/Commands/Repl/ReplCommand.cs +++ b/src/Cli/Commands/Repl/ReplCommand.cs @@ -221,9 +221,13 @@ protected override async Task ExecuteAsync( toolsByCategory["Shell"] = shellFunctions.Where(f => CoreShellTools.Contains(f.Name)).ToList(); toolsByCategory["Git"] = gitFunctions.Where(f => CoreGitTools.Contains(f.Name)).ToList(); - (skillsPlugin, skillsCatalog) = ReplSkillsLoader.BuildSkills(); + var skillsResult = ReplSkillsLoader.BuildSkillsDetailed(ReplSkillsLoader.GetDefaultSearchDirs()); + skillsPlugin = skillsResult.Plugin; + skillsCatalog = skillsResult.CatalogBlock; if (skillsPlugin is not null) toolsByCategory["Skills"] = PluginRegistry.GetFunctionsFromObject(skillsPlugin).ToList(); + foreach (var warning in skillsResult.Warnings) + AnsiConsole.MarkupLine($"[yellow]⚠[/] {Markup.Escape(warning)}"); } var initialTools = toolsByCategory.Values.SelectMany(v => v).ToList(); diff --git a/src/Cli/Commands/Repl/ReplSkillsLoader.cs b/src/Cli/Commands/Repl/ReplSkillsLoader.cs index e4795be6..5907f742 100644 --- a/src/Cli/Commands/Repl/ReplSkillsLoader.cs +++ b/src/Cli/Commands/Repl/ReplSkillsLoader.cs @@ -1,8 +1,24 @@ +using System.Text; using fuseraft.Core; +using fuseraft.Core.Skills; using fuseraft.Infrastructure.Plugins; namespace fuseraft.Cli.Commands.Repl; +/// Full result of a skill-directory scan: the catalog plugin plus any diagnostics. +/// The assembled , or null when no skills were found. +/// Catalog text for the REPL system prompt, or null when no skills were found. +/// +/// Human-readable reasons a discovered SKILL.md was skipped — always because it declared +/// a name:, description:, or compatibility: field that violates the Agent +/// Skills specification (). A skill that omits +/// frontmatter entirely is never skipped — see the "leniency" note on . +/// +internal sealed record SkillsLoadResult( + SkillsPlugin? Plugin, + string? CatalogBlock, + IReadOnlyList Warnings); + /// /// Scans skill directories, parses SKILL.md frontmatter, and assembles the /// instance and catalog block injected into the REPL @@ -10,6 +26,11 @@ namespace fuseraft.Cli.Commands.Repl; /// internal static class ReplSkillsLoader { + // Matches orchestration's AgentFileSkillsSource search depth: root (0), skill dir (1), + // an optional one level of vendor namespacing (2). Bounded so a search dir pointed at a + // large or cyclic tree can't cause a runaway scan. + private const int MaxSkillSearchDepth = 2; + /// /// Returns the priority-ordered list of directories to scan for skills in a /// normal REPL session (project-local → user-global → install-bundled). @@ -35,29 +56,43 @@ internal static (SkillsPlugin? Plugin, string? CatalogBlock) BuildSkills() => BuildSkills(GetDefaultSearchDirs()); /// - /// Scans for SKILL.md files, builds a - /// slug-to-directory map (first occurrence across dirs wins), and returns a - /// together with a catalog string suitable for - /// appending to the REPL system prompt. + /// Scans for SKILL.md files and returns a + /// together with a catalog string suitable for appending to the + /// REPL system prompt. Discards any skip warnings — see + /// for a caller that wants them. /// - /// Returns (null, null) when no skills are found. /// - /// Inaccessible directories are silently skipped so a permissions error on - /// one dir does not block skills from other dirs. + /// Leniency: a SKILL.md with no frontmatter at all (or frontmatter with none + /// of the recognized fields) is still loaded, using its directory name as the slug — this + /// REPL surface does not require a name:/description: field the way fuseraft's + /// orchestration skills provider does. But when a name:, description:, or + /// compatibility: field is declared, it is validated against the same rules + /// orchestration enforces, and a violation (most commonly name: not matching the + /// directory name) skips the skill — silently accepting it here would let a skill work in + /// the REPL while remaining invisible to fuseraft run orchestration sessions. /// /// internal static (SkillsPlugin? Plugin, string? CatalogBlock) BuildSkills(IEnumerable searchDirs) { - // slug → directory containing SKILL.md; first occurrence wins. - var skillDirs = new Dictionary(StringComparer.OrdinalIgnoreCase); - var descriptions = new Dictionary(StringComparer.OrdinalIgnoreCase); + var result = BuildSkillsDetailed(searchDirs); + return (result.Plugin, result.CatalogBlock); + } + + /// Same scan as , but also returns skip warnings. + internal static SkillsLoadResult BuildSkillsDetailed(IEnumerable searchDirs) + { + // slug → directory containing SKILL.md; first occurrence across searchDirs wins. + var skillDirs = new Dictionary(StringComparer.OrdinalIgnoreCase); + var descriptions = new Dictionary(StringComparer.OrdinalIgnoreCase); + var compatibility = new Dictionary(StringComparer.OrdinalIgnoreCase); + var warnings = new List(); foreach (var searchDir in searchDirs.Where(Directory.Exists)) { - IEnumerable skillMds; + List skillMds; try { - skillMds = Directory.EnumerateFiles(searchDir, "SKILL.md", SearchOption.AllDirectories); + skillMds = FindSkillMdFiles(searchDir); } catch (UnauthorizedAccessException) { continue; } catch (IOException) { continue; } @@ -66,56 +101,155 @@ internal static (SkillsPlugin? Plugin, string? CatalogBlock) BuildSkills(IEnumer { var skillDir = Path.GetDirectoryName(skillMd); if (skillDir is null) continue; - var slug = Path.GetFileName(skillDir); - if (string.IsNullOrEmpty(slug) || skillDirs.ContainsKey(slug)) continue; + var dirName = Path.GetFileName(skillDir); + if (string.IsNullOrEmpty(dirName) || skillDirs.ContainsKey(dirName)) continue; + + string content; + try + { + content = File.ReadAllText(skillMd); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { continue; } - skillDirs[slug] = skillDir; - descriptions[slug] = ParseSkillDescription(skillMd); + if (!TryValidateFrontmatter(content, skillDir, dirName, warnings, + out var description, out var compat)) + continue; + + skillDirs[dirName] = skillDir; + descriptions[dirName] = description; + compatibility[dirName] = compat; } } - if (skillDirs.Count == 0) return (null, null); + if (skillDirs.Count == 0) return new SkillsLoadResult(null, null, warnings); - var sb = new System.Text.StringBuilder(); + var sb = new StringBuilder(); sb.AppendLine("## SKILLS available"); foreach (var slug in skillDirs.Keys.OrderBy(s => s, StringComparer.OrdinalIgnoreCase)) { - var desc = descriptions.GetValueOrDefault(slug); - sb.AppendLine(!string.IsNullOrWhiteSpace(desc) ? $"- {slug}: {desc}" : $"- {slug}"); + var desc = descriptions.GetValueOrDefault(slug); + var compat = compatibility.GetValueOrDefault(slug); + var line = !string.IsNullOrWhiteSpace(desc) ? $"- {slug}: {desc}" : $"- {slug}"; + if (!string.IsNullOrWhiteSpace(compat)) + line += $" [requires: {compat}]"; + sb.AppendLine(line); } sb.AppendLine(); sb.Append("Call load_skill(\"\") to get full step-by-step instructions before applying a skill."); - return (new SkillsPlugin(skillDirs), sb.ToString()); + return new SkillsLoadResult(new SkillsPlugin(skillDirs), sb.ToString(), warnings); + } + + /// + /// Parses 's frontmatter and validates any spec-covered field that + /// is actually present. Returns false (and appends a warning) only when a declared + /// field violates the spec — a skill with no frontmatter, or frontmatter missing these + /// fields entirely, always passes (see the leniency note on ). + /// + private static bool TryValidateFrontmatter( + string content, string skillDir, string dirName, List warnings, + out string? description, out string? compatibility) + { + description = null; + compatibility = null; + + var fm = SkillFrontmatterSpec.TryParse(content); + if (fm is null) return true; + + if (!string.IsNullOrEmpty(fm.Name)) + { + if (!SkillFrontmatterSpec.ValidateName(fm.Name, out var nameReason)) + { + warnings.Add($"Skipped skill at '{skillDir}': {nameReason}"); + return false; + } + if (!string.Equals(fm.Name, dirName, StringComparison.Ordinal)) + { + warnings.Add( + $"Skipped skill at '{skillDir}': name '{fm.Name}' does not match its directory " + + $"name '{dirName}' (this skill would also be invisible to 'fuseraft run' orchestration sessions)."); + return false; + } + } + + if (!string.IsNullOrEmpty(fm.Description)) + { + if (!SkillFrontmatterSpec.ValidateDescription(fm.Description, out var descReason)) + { + warnings.Add($"Skipped skill at '{skillDir}': {descReason}"); + return false; + } + description = fm.Description; + } + + if (!SkillFrontmatterSpec.ValidateCompatibility(fm.Compatibility, out var compatReason)) + { + warnings.Add($"Skipped skill at '{skillDir}': {compatReason}"); + return false; + } + compatibility = fm.Compatibility; + + return true; + } + + /// + /// Finds every SKILL.md under , recursing at most + /// levels and refusing to follow symlinked directories — + /// unbounded, symlink-following recursion could otherwise be tricked (via a symlink planted + /// in a project-controlled search dir) into scanning arbitrary parts of the filesystem, or + /// hang on a symlink cycle. Once a directory yields a SKILL.md, its subdirectories are + /// treated as part of that skill (references/scripts/assets), not as independent skill roots. + /// + private static List FindSkillMdFiles(string root) + { + var results = new List(); + FindSkillMdFiles(root, results, depth: 0); + return results; + } + + private static void FindSkillMdFiles(string directory, List results, int depth) + { + var candidate = Path.Combine(directory, "SKILL.md"); + if (File.Exists(candidate)) + { + if (!SkillPathGuard.IsReparsePoint(candidate)) + results.Add(candidate); + return; + } + + if (depth >= MaxSkillSearchDepth) return; + + IEnumerable subdirs; + try + { + subdirs = Directory.EnumerateDirectories(directory); + } + catch (Exception ex) when (ex is UnauthorizedAccessException or IOException) { return; } + + foreach (var sub in subdirs) + { + if (SkillPathGuard.IsReparsePoint(sub)) continue; + FindSkillMdFiles(sub, results, depth + 1); + } } /// /// Reads only the description: field from a SKILL.md YAML frontmatter block. /// Returns null when the field is absent, empty, or the file is unreadable. + /// Kept as a thin wrapper over for callers that only need + /// the description of a single known file. /// internal static string? ParseSkillDescription(string skillMdPath) { try { - var inFrontmatter = false; - foreach (var line in File.ReadLines(skillMdPath)) - { - var trimmed = line.Trim(); - if (trimmed == "---") - { - if (!inFrontmatter) { inFrontmatter = true; continue; } - break; // closing delimiter - } - if (!inFrontmatter) break; // no opening delimiter on first line - - if (trimmed.StartsWith("description:", StringComparison.OrdinalIgnoreCase)) - { - var value = trimmed["description:".Length..].Trim().Trim('"').Trim('\''); - return string.IsNullOrWhiteSpace(value) ? null : value; - } - } + var content = File.ReadAllText(skillMdPath); + var fm = SkillFrontmatterSpec.TryParse(content); + return string.IsNullOrWhiteSpace(fm?.Description) ? null : fm.Description; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { return null; } - catch { return null; } } } diff --git a/src/Cli/Commands/Skills/SkillsAddCommand.cs b/src/Cli/Commands/Skills/SkillsAddCommand.cs index 22a3cdf0..83248868 100644 --- a/src/Cli/Commands/Skills/SkillsAddCommand.cs +++ b/src/Cli/Commands/Skills/SkillsAddCommand.cs @@ -2,6 +2,7 @@ using Spectre.Console; using Spectre.Console.Cli; using fuseraft.Core; +using fuseraft.Core.Skills; using fuseraft.Orchestration; namespace fuseraft.Cli.Commands.Skills; @@ -54,6 +55,26 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsAd return 1; } + if (slug.Length > SkillFrontmatterSpec.MaxNameLength) + { + slug = slug[..SkillFrontmatterSpec.MaxNameLength].TrimEnd('-'); + AnsiConsole.MarkupLine( + $"[yellow]⚠[/] Derived name exceeds {SkillFrontmatterSpec.MaxNameLength} characters; truncated to [bold]{Markup.Escape(slug)}[/]."); + } + + // Guarantee the installed file's 'name:' field matches the directory it's installed + // under — a raw name that needed slugifying (spaces, uppercase, ...) would otherwise + // leave the two disagreeing, which works fine in the REPL's lenient loader but is + // silently dropped by fuseraft's orchestration skills provider. + content = SkillsHelpers.CanonicalizeName(content, slug); + + if (!SkillFrontmatterSpec.ValidateDescription(SkillsHelpers.ExtractDescription(content), out var descReason)) + { + AnsiConsole.MarkupLine( + $"[yellow]⚠[/] {Markup.Escape(descReason!)} " + + "This skill will work in the REPL but 'fuseraft run' orchestration sessions will silently drop it."); + } + var destDir = Path.Combine(FuseraftPaths.GlobalSkills, slug); var destPath = Path.Combine(destDir, "SKILL.md"); var isUpdate = File.Exists(destPath); @@ -66,10 +87,8 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsAd // skill's own instructions point to (load_skill/read_skill_resource/run_skill_script). SkillsHelpers.CopySkillDirectory(sourceSkillDir, destDir); } - else - { - await File.WriteAllTextAsync(destPath, content, cancellationToken); - } + // Write (or overwrite, if just copied) SKILL.md with the possibly name-canonicalized content. + await File.WriteAllTextAsync(destPath, content, cancellationToken); await using var index = new SkillIndex(); try diff --git a/src/Cli/Commands/Skills/SkillsHelpers.cs b/src/Cli/Commands/Skills/SkillsHelpers.cs index 157aa748..fc15811e 100644 --- a/src/Cli/Commands/Skills/SkillsHelpers.cs +++ b/src/Cli/Commands/Skills/SkillsHelpers.cs @@ -1,33 +1,37 @@ -using System.Text.RegularExpressions; +using fuseraft.Core.Skills; namespace fuseraft.Cli.Commands.Skills; internal static class SkillsHelpers { - private static readonly Regex NameFrontmatter = - new(@"^name:\s*(.+)$", RegexOptions.Multiline | RegexOptions.IgnoreCase); - - private static readonly Regex DescriptionFrontmatter = - new(@"^description:\s*(.+)$", RegexOptions.Multiline | RegexOptions.IgnoreCase); - + /// Extracts the slugified name: field, or null when absent/empty. internal static string? ExtractSlug(string content) { - var m = NameFrontmatter.Match(content); - if (!m.Success) return null; - var name = m.Groups[1].Value.Trim().Trim('"').Trim('\''); + var name = SkillFrontmatterSpec.TryParse(content)?.Name; return string.IsNullOrWhiteSpace(name) ? null : ToSlug(name); } - internal static string ExtractDescription(string content) + internal static string ExtractDescription(string content) => + SkillFrontmatterSpec.TryParse(content)?.Description ?? string.Empty; + + internal static string ToSlug(string name) => SkillFrontmatterSpec.ToSlug(name); + + /// + /// Rewrites 's name: frontmatter field to + /// when it isn't already exactly that value. Ensures a skill + /// installed under <slug>/SKILL.md always has a matching name: field — + /// without this, a raw name that needed slugifying (spaces, uppercase, etc.) would leave the + /// installed file internally inconsistent: fine in the REPL's lenient loader, but silently + /// dropped by fuseraft's orchestration skills provider, which requires an exact match. + /// + internal static string CanonicalizeName(string content, string slug) { - var m = DescriptionFrontmatter.Match(content); - if (!m.Success) return string.Empty; - return m.Groups[1].Value.Trim().Trim('"').Trim('\''); + var currentName = SkillFrontmatterSpec.TryParse(content)?.Name; + return string.Equals(currentName, slug, StringComparison.Ordinal) + ? content + : SkillFrontmatterSpec.WithCanonicalName(content, slug); } - internal static string ToSlug(string name) => - Regex.Replace(name.ToLowerInvariant().Trim(), @"[^a-z0-9]+", "-").Trim('-'); - /// /// Recursively copies every file under into /// , preserving relative subdirectory structure and creating diff --git a/src/Cli/Commands/Skills/SkillsListCommand.cs b/src/Cli/Commands/Skills/SkillsListCommand.cs index 4bbdde02..f818f502 100644 --- a/src/Cli/Commands/Skills/SkillsListCommand.cs +++ b/src/Cli/Commands/Skills/SkillsListCommand.cs @@ -2,6 +2,7 @@ using Spectre.Console; using Spectre.Console.Cli; using fuseraft.Core; +using fuseraft.Core.Skills; namespace fuseraft.Cli.Commands.Skills; @@ -21,15 +22,17 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsLi return 0; } - var entries = new List<(string Slug, string Description)>(); + var entries = new List<(string Slug, string Description, string? Compatibility, bool Valid)>(); foreach (var dir in Directory.EnumerateDirectories(root).OrderBy(d => d)) { var mdPath = Path.Combine(dir, "SKILL.md"); if (!File.Exists(mdPath)) continue; - var content = await File.ReadAllTextAsync(mdPath, cancellationToken); - var slug = Path.GetFileName(dir); - var desc = SkillsHelpers.ExtractDescription(content); - entries.Add((slug, desc)); + var content = await File.ReadAllTextAsync(mdPath, cancellationToken); + var slug = Path.GetFileName(dir); + var frontmatter = SkillFrontmatterSpec.TryParse(content); + var desc = frontmatter?.Description ?? string.Empty; + var valid = SkillFrontmatterSpec.Validate(frontmatter, slug).Count == 0; + entries.Add((slug, desc, frontmatter?.Compatibility, valid)); } if (entries.Count == 0) @@ -41,13 +44,24 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsLi var table = new Table() .Border(TableBorder.Simple) .AddColumn(new TableColumn("[bold]Slug[/]")) - .AddColumn(new TableColumn("[bold]Description[/]")); + .AddColumn(new TableColumn("[bold]Description[/]")) + .AddColumn(new TableColumn("[bold]Requires[/]")) + .AddColumn(new TableColumn("[bold]Spec[/]")); - foreach (var (slug, desc) in entries) - table.AddRow(Markup.Escape(slug), Markup.Escape(desc)); + foreach (var (slug, desc, compatibility, valid) in entries) + { + var specMark = valid ? "[green]✓[/]" : "[red]✗[/]"; + table.AddRow( + Markup.Escape(slug), + Markup.Escape(desc), + Markup.Escape(compatibility ?? ""), + specMark); + } AnsiConsole.Write(table); AnsiConsole.MarkupLine($"[dim]{entries.Count} skill(s) in {Markup.Escape(root)}[/]"); + if (entries.Any(e => !e.Valid)) + AnsiConsole.MarkupLine("[dim]Run [bold]fuseraft skills validate[/] for details on the ✗ entries.[/]"); return 0; } } diff --git a/src/Cli/Commands/Skills/SkillsValidateCommand.cs b/src/Cli/Commands/Skills/SkillsValidateCommand.cs new file mode 100644 index 00000000..bde68c01 --- /dev/null +++ b/src/Cli/Commands/Skills/SkillsValidateCommand.cs @@ -0,0 +1,88 @@ +using System.ComponentModel; +using Spectre.Console; +using Spectre.Console.Cli; +using fuseraft.Core; +using fuseraft.Core.Skills; + +namespace fuseraft.Cli.Commands.Skills; + +// fuseraft skills validate [path] + +public sealed class SkillsValidateSettings : CommandSettings +{ + [CommandArgument(0, "[path]")] + [Description("Path to a skill directory to validate. Omit to validate every skill in ~/.fuseraft/skills.")] + public string? Path { get; set; } +} + +/// +/// fuseraft's equivalent of the skills-ref validate tool the Agent Skills specification +/// () recommends authors run before +/// shipping a skill — checks a SKILL.md's frontmatter against every naming and field-length rule +/// the spec defines, using the exact same validator fuseraft's orchestration skills provider +/// applies at load time. +/// +public sealed class SkillsValidateCommand : AsyncCommand +{ + protected override async Task ExecuteAsync(CommandContext context, SkillsValidateSettings settings, CancellationToken cancellationToken) + { + List skillDirs; + + if (!string.IsNullOrWhiteSpace(settings.Path)) + { + var dir = FuseraftPaths.ExpandPath(settings.Path); + if (!Directory.Exists(dir)) + { + AnsiConsole.MarkupLine($"[red]✗ Not a directory: {Markup.Escape(settings.Path)}[/]"); + return 1; + } + skillDirs = [dir.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)]; + } + else + { + var root = FuseraftPaths.GlobalSkills; + if (!Directory.Exists(root)) + { + AnsiConsole.MarkupLine("[dim]No skills installed. Use [bold]fuseraft skills add [/] to add one.[/]"); + return 0; + } + skillDirs = Directory.EnumerateDirectories(root).OrderBy(d => d, StringComparer.OrdinalIgnoreCase).ToList(); + } + + var allValid = true; + foreach (var dir in skillDirs) + { + var name = Path.GetFileName(dir); + var skillMd = Path.Combine(dir, "SKILL.md"); + + if (!File.Exists(skillMd)) + { + allValid = false; + AnsiConsole.MarkupLine($"[red]✗[/] [bold]{Markup.Escape(name)}[/] — no SKILL.md found"); + continue; + } + + var content = await File.ReadAllTextAsync(skillMd, cancellationToken); + var frontmatter = SkillFrontmatterSpec.TryParse(content); + var violations = SkillFrontmatterSpec.Validate(frontmatter, name); + + if (violations.Count == 0) + { + AnsiConsole.MarkupLine($"[green]✓[/] [bold]{Markup.Escape(name)}[/]"); + continue; + } + + allValid = false; + AnsiConsole.MarkupLine($"[red]✗[/] [bold]{Markup.Escape(name)}[/]"); + foreach (var violation in violations) + AnsiConsole.MarkupLine($" [red]•[/] {Markup.Escape(violation)}"); + } + + if (!allValid) + AnsiConsole.MarkupLine( + "\n[yellow]A skill listed above works fine in the REPL's lenient loader but is silently dropped " + + "by 'fuseraft run' orchestration sessions, which require full spec conformance.[/]"); + + return allValid ? 0 : 1; + } +} diff --git a/src/Core/Skills/SkillFrontmatter.cs b/src/Core/Skills/SkillFrontmatter.cs new file mode 100644 index 00000000..1d277dc5 --- /dev/null +++ b/src/Core/Skills/SkillFrontmatter.cs @@ -0,0 +1,266 @@ +using System.Text.RegularExpressions; + +namespace fuseraft.Core.Skills; + +/// +/// The YAML frontmatter fields of a SKILL.md file as defined by the +/// Agent Skills specification. +/// +/// Raw name: value, or empty string if absent. Not guaranteed valid — use . +/// Raw description: value, or empty string if absent. +/// Optional license: value. +/// Optional compatibility: value. +/// Optional allowed-tools: value (space-separated, experimental per spec). +/// Optional metadata: map of string keys to string values. +public sealed record SkillFrontmatter( + string Name, + string Description, + string? License, + string? Compatibility, + string? AllowedTools, + IReadOnlyDictionary? Metadata); + +/// +/// Single source of truth for parsing and validating SKILL.md frontmatter against the +/// Agent Skills specification (). +/// +/// +/// fuseraft has two separate skill-loading surfaces — the REPL's own hand-rolled loader +/// (ReplSkillsLoader/SkillsPlugin) and orchestration's Microsoft.Agents.AI +/// skills provider — that historically diverged in what they accepted. This type mirrors the +/// validation rules of Microsoft's AgentSkillFrontmatter exactly (same length limits, same +/// name regex) so both surfaces treat a given SKILL.md identically, and so the CLI-side commands +/// (skills add, skills validate, skill curation) can enforce the same rules before +/// ever writing a file to disk. +/// +/// +public static class SkillFrontmatterSpec +{ + public const int MaxNameLength = 64; + public const int MaxDescriptionLength = 1024; + public const int MaxCompatibilityLength = 500; + + private static readonly TimeSpan RegexTimeout = TimeSpan.FromSeconds(2); + + // Lowercase letters, numbers, and hyphens only; no leading/trailing/consecutive hyphens. + private static readonly Regex ValidNameRegex = + new(@"^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$", RegexOptions.Compiled, RegexTimeout); + + // Matches the YAML frontmatter block delimited by "---" lines. Callers strip a leading + // UTF-8 BOM (via TrimBom) before matching, since some editors prepend one. + private static readonly Regex FrontmatterBlock = + new(@"\A^---\s*$(.*?)^---\s*$", RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled, RegexTimeout); + + // Matches a top-level "key: value" line (no leading indentation) — double-quoted, + // single-quoted, or bare scalar values. + private static readonly Regex TopLevelKeyValue = + new(@"^([A-Za-z][\w-]*)\s*:[ \t]*(?:""([^""]*)""|'([^']*)'|(\S.*?))?\s*$", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); + + // Matches a "metadata:" line followed by one or more indented sub-lines. + private static readonly Regex MetadataBlock = + new(@"^metadata\s*:\s*$\r?\n((?:[ \t]+\S.*(?:\r?\n|\z))+)", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); + + // Matches an indented "key: value" line within a metadata block. + private static readonly Regex IndentedKeyValue = + new(@"^[ \t]+([A-Za-z][\w-]*)\s*:[ \t]*(?:""([^""]*)""|'([^']*)'|(\S.*?))?\s*$", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); + + private static readonly Regex SlugSanitizer = new(@"[^a-z0-9]+", RegexOptions.Compiled, RegexTimeout); + + /// + /// Parses the YAML frontmatter block from a SKILL.md file's content. Returns null + /// when there is no frontmatter block at all, or the block contains none of the recognized + /// fields. This is a raw parse — it does not validate the values; call + /// to check spec conformance. + /// + public static SkillFrontmatter? TryParse(string? content) + { + if (string.IsNullOrEmpty(content)) return null; + + Match block; + try { block = FrontmatterBlock.Match(TrimBom(content)); } + catch (RegexMatchTimeoutException) { return null; } + if (!block.Success) return null; + + var yaml = block.Groups[1].Value; + + string? name = null, description = null, license = null, compatibility = null, allowedTools = null; + foreach (Match m in TopLevelKeyValue.Matches(yaml)) + { + var value = ExtractValue(m); + switch (m.Groups[1].Value.ToLowerInvariant()) + { + case "name": name = value; break; + case "description": description = value; break; + case "license": license = value; break; + case "compatibility": compatibility = value; break; + case "allowed-tools": allowedTools = value; break; + } + } + + Dictionary? metadata = null; + var metadataMatch = MetadataBlock.Match(yaml); + if (metadataMatch.Success) + { + metadata = new Dictionary(StringComparer.Ordinal); + foreach (Match m in IndentedKeyValue.Matches(metadataMatch.Groups[1].Value)) + metadata[m.Groups[1].Value] = ExtractValue(m); + } + + if (string.IsNullOrEmpty(name) && string.IsNullOrEmpty(description) && + license is null && compatibility is null && allowedTools is null && metadata is null) + return null; + + return new SkillFrontmatter( + name ?? string.Empty, + description ?? string.Empty, + license, + compatibility, + allowedTools, + metadata); + } + + private static string ExtractValue(Match m) => + (m.Groups[2].Success ? m.Groups[2].Value + : m.Groups[3].Success ? m.Groups[3].Value + : m.Groups[4].Success ? m.Groups[4].Value + : string.Empty).Trim(); + + /// + /// Validates a skill name: 1-64 characters, lowercase letters/numbers/hyphens only, + /// no leading/trailing/consecutive hyphens. + /// + public static bool ValidateName(string? name, out string? reason) + { + if (string.IsNullOrWhiteSpace(name)) + { + reason = "Skill name is required."; + return false; + } + if (name.Length > MaxNameLength) + { + reason = $"Skill name must be {MaxNameLength} characters or fewer."; + return false; + } + if (!ValidNameRegex.IsMatch(name)) + { + reason = "Skill name must use only lowercase letters, numbers, and hyphens, and must not start or end with a hyphen or contain consecutive hyphens."; + return false; + } + reason = null; + return true; + } + + /// Validates a skill description: required, non-empty, 1-1024 characters. + public static bool ValidateDescription(string? description, out string? reason) + { + if (string.IsNullOrWhiteSpace(description)) + { + reason = "Skill description is required."; + return false; + } + if (description.Length > MaxDescriptionLength) + { + reason = $"Skill description must be {MaxDescriptionLength} characters or fewer."; + return false; + } + reason = null; + return true; + } + + /// Validates the optional compatibility field: at most 500 characters. + public static bool ValidateCompatibility(string? compatibility, out string? reason) + { + if (compatibility?.Length > MaxCompatibilityLength) + { + reason = $"Skill compatibility must be {MaxCompatibilityLength} characters or fewer."; + return false; + } + reason = null; + return true; + } + + /// + /// Full conformance check for a parsed frontmatter against a specific skill directory name — + /// the same checks fuseraft's orchestration skills provider applies. Returns every violation + /// found (empty list means fully compliant). + /// + public static IReadOnlyList Validate(SkillFrontmatter? frontmatter, string directoryName) + { + var violations = new List(); + + if (frontmatter is null) + { + violations.Add("No YAML frontmatter block found (SKILL.md must start with a '---' delimited block)."); + return violations; + } + + if (!ValidateName(frontmatter.Name, out var nameReason)) + { + violations.Add(nameReason!); + } + else if (!string.Equals(frontmatter.Name, directoryName, StringComparison.Ordinal)) + { + violations.Add($"'name: {frontmatter.Name}' does not match parent directory name '{directoryName}'."); + } + + if (!ValidateDescription(frontmatter.Description, out var descReason)) + violations.Add(descReason!); + + if (!ValidateCompatibility(frontmatter.Compatibility, out var compatReason)) + violations.Add(compatReason!); + + return violations; + } + + /// + /// Converts an arbitrary string into a spec-valid slug: lowercase, non-alphanumeric runs + /// collapsed to single hyphens, no leading/trailing hyphens. Used to derive a directory + /// name (and, after normalization, the name: field) from a skill's raw title. + /// + public static string ToSlug(string name) => + SlugSanitizer.Replace((name ?? string.Empty).ToLowerInvariant().Trim(), "-").Trim('-'); + + /// + /// Rewrites the name: line of a SKILL.md's frontmatter to , + /// leaving the rest of the file untouched. Used when installing or curating a skill whose + /// original name: field doesn't match the slug it will be installed under — without + /// this, the file on disk and its own directory name would disagree, which fuseraft's + /// orchestration skills provider treats as an invalid skill and silently drops. + /// Appends a name: line to the frontmatter block if one was missing entirely. + /// + public static string WithCanonicalName(string content, string slug) + { + var trimmed = TrimBom(content); + Match block; + try { block = FrontmatterBlock.Match(trimmed); } + catch (RegexMatchTimeoutException) { return content; } + if (!block.Success) return content; + + var yaml = block.Groups[1].Value; + var nameLine = $"name: {slug}"; + + string newYaml; + var nameMatch = TopLevelKeyValue.Matches(yaml) + .Cast() + .FirstOrDefault(m => string.Equals(m.Groups[1].Value, "name", StringComparison.OrdinalIgnoreCase)); + + if (nameMatch is not null) + { + newYaml = yaml[..nameMatch.Index] + nameLine + yaml[(nameMatch.Index + nameMatch.Length)..]; + } + else + { + // The captured yaml group starts right after "---" and before its own trailing + // newline (the '$' anchor is zero-width), so it always begins with '\n' — restore + // that leading newline here to keep "name:" on its own line after "---". + newYaml = "\n" + nameLine + "\n" + yaml.TrimStart('\n'); + } + + return trimmed[..block.Groups[1].Index] + newYaml + trimmed[(block.Groups[1].Index + block.Groups[1].Length)..]; + } + + /// Strips a leading UTF-8 BOM, which some editors prepend and which would + /// otherwise prevent the frontmatter block regex from matching at position 0. + private static string TrimBom(string content) => + content.Length > 0 && content[0] == '\uFEFF' ? content[1..] : content; +} diff --git a/src/Core/Skills/SkillPathGuard.cs b/src/Core/Skills/SkillPathGuard.cs new file mode 100644 index 00000000..4fe334a9 --- /dev/null +++ b/src/Core/Skills/SkillPathGuard.cs @@ -0,0 +1,71 @@ +namespace fuseraft.Core.Skills; + +/// +/// Path-containment and symlink-escape checks for resolving a model-supplied relative path +/// against a trusted skill directory root. +/// +/// +/// only normalizes a path lexically (collapsing +/// .. segments) — it does not resolve symbolic links. A lexical containment check alone +/// (resolved.StartsWith(skillRoot)) is therefore not sufficient: a symlink planted +/// anywhere inside a skill directory (e.g. references symlinked to /etc, or a +/// single file symlinked to ~/.ssh/id_rsa) would pass that check while actually reading +/// or executing a file outside the skill. This mirrors the symlink-escape protection in +/// Microsoft's AgentFileSkillsSource, which fuseraft's orchestration skills provider is +/// built on. +/// +/// +public static class SkillPathGuard +{ + /// + /// Resolves against and + /// confirms the result stays inside the root with no symlinked path segment along the way. + /// + public static bool TryResolveSafePath( + string skillRoot, + string relativePath, + out string fullPath, + out string? reason) + { + var root = Path.GetFullPath(skillRoot); + var rootWithSep = root.EndsWith(Path.DirectorySeparatorChar) ? root : root + Path.DirectorySeparatorChar; + + fullPath = Path.GetFullPath(Path.Combine(root, relativePath)); + if (!fullPath.StartsWith(rootWithSep, StringComparison.Ordinal)) + { + reason = "path is outside the skill directory."; + return false; + } + + var relative = Path.GetRelativePath(root, fullPath); + var current = root; + foreach (var segment in relative.Split(Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries)) + { + current = Path.Combine(current, segment); + if (!File.Exists(current) && !Directory.Exists(current)) + continue; // let the caller's own not-found handling report this + + FileAttributes attrs; + try { attrs = File.GetAttributes(current); } + catch (IOException) { continue; } + catch (UnauthorizedAccessException) { continue; } + + if (attrs.HasFlag(FileAttributes.ReparsePoint)) + { + reason = $"'{segment}' is a symlink; skill paths may not traverse symlinks."; + return false; + } + } + + reason = null; + return true; + } + + /// True when exists and is a symlink/reparse point. + public static bool IsReparsePoint(string path) + { + try { return File.GetAttributes(path).HasFlag(FileAttributes.ReparsePoint); } + catch (IOException) { return false; } + catch (UnauthorizedAccessException) { return false; } + } +} diff --git a/src/Infrastructure/Plugins/SkillsPlugin.cs b/src/Infrastructure/Plugins/SkillsPlugin.cs index 21b249db..100dad91 100644 --- a/src/Infrastructure/Plugins/SkillsPlugin.cs +++ b/src/Infrastructure/Plugins/SkillsPlugin.cs @@ -1,6 +1,7 @@ using System.ComponentModel; using System.Diagnostics; using System.Runtime.InteropServices; +using fuseraft.Core.Skills; namespace fuseraft.Infrastructure.Plugins; @@ -71,13 +72,12 @@ public async Task ReadSkillResourceAsync( if (string.IsNullOrWhiteSpace(resourcePath)) return PluginResult.Error("Resource path must not be empty."); - // Resolve against the skill directory and confirm the result stays inside it — - // resourcePath comes from the model, so an absolute path or "../" sequence must - // not be able to escape to arbitrary files on disk. - var skillRoot = Path.GetFullPath(dir) + Path.DirectorySeparatorChar; - var fullPath = Path.GetFullPath(Path.Combine(dir, resourcePath)); - if (!fullPath.StartsWith(skillRoot, StringComparison.Ordinal)) - return PluginResult.Error($"'{resourcePath}' is outside the skill directory."); + // Resolve against the skill directory and confirm the result stays inside it, with no + // symlinked path segment along the way — resourcePath comes from the model, so an + // absolute path, a "../" sequence, or a symlink planted in the skill directory must not + // be able to escape to arbitrary files on disk. + if (!SkillPathGuard.TryResolveSafePath(dir, resourcePath, out var fullPath, out var reason)) + return PluginResult.Error($"'{resourcePath}' {reason}"); if (!File.Exists(fullPath)) return PluginResult.NotFound($"Resource '{resourcePath}' not found in skill '{skill}'."); @@ -103,11 +103,9 @@ public async Task RunSkillScriptAsync( return PluginResult.NotFound($"No skill '{skill}'."); // Resolve against the skill directory and confirm the result stays inside it — same - // containment check as ReadSkillResourceAsync, since 'script' comes from the model. - var skillRoot = Path.GetFullPath(dir) + Path.DirectorySeparatorChar; - var scriptPath = Path.GetFullPath(Path.Combine(dir, script)); - if (!scriptPath.StartsWith(skillRoot, StringComparison.Ordinal)) - return PluginResult.Error($"'{script}' is outside the skill directory."); + // containment/symlink check as ReadSkillResourceAsync, since 'script' comes from the model. + if (!SkillPathGuard.TryResolveSafePath(dir, script, out var scriptPath, out var reason)) + return PluginResult.Error($"'{script}' {reason}"); if (!File.Exists(scriptPath)) return PluginResult.NotFound($"Script '{script}' not found in skill '{skill}'."); diff --git a/src/Orchestration/Skills/SkillCurator.cs b/src/Orchestration/Skills/SkillCurator.cs index 12dc48c7..f8a9330e 100644 --- a/src/Orchestration/Skills/SkillCurator.cs +++ b/src/Orchestration/Skills/SkillCurator.cs @@ -6,6 +6,7 @@ using Microsoft.Extensions.Logging; using fuseraft.Core; using fuseraft.Core.Models; +using fuseraft.Core.Skills; namespace fuseraft.Orchestration.Skills; @@ -67,9 +68,6 @@ public sealed class SkillCurator( private static readonly Regex SkillBlock = new(@"(.*?)", RegexOptions.Singleline | RegexOptions.IgnoreCase); - private static readonly Regex NameFrontmatter = - new(@"^name:\s*(.+)$", RegexOptions.Multiline | RegexOptions.IgnoreCase); - private static readonly JsonSerializerOptions LogJsonOpts = new() { PropertyNamingPolicy = JsonNamingPolicy.SnakeCaseLower, @@ -167,8 +165,9 @@ public async Task RunAsync( } var skillContent = match.Groups[1].Value.Trim(); - var nameMatch = NameFrontmatter.Match(skillContent); - if (!nameMatch.Success) + var frontmatter = SkillFrontmatterSpec.TryParse(skillContent); + + if (string.IsNullOrWhiteSpace(frontmatter?.Name)) { const string noNameReason = "SKILL block is missing the 'name:' frontmatter field."; logger.LogWarning( @@ -183,8 +182,41 @@ public async Task RunAsync( return failed; } - var name = nameMatch.Groups[1].Value.Trim().Trim('"').Trim('\''); - var slug = ToSlug(name); + if (!SkillFrontmatterSpec.ValidateDescription(frontmatter.Description, out var descReason)) + { + logger.LogWarning( + "Skill curation failed — session={Session} reason={Reason}", + checkpoint.SessionId, descReason); + var failed = new SkillCurationResult( + SkillCurationOutcome.Failed, + FailureReason: descReason, + TurnsDigested: digestTurns, + Model: modelId); + await AppendCurationLogAsync(checkpoint.SessionId, failed, source, ct); + return failed; + } + + var slug = SkillFrontmatterSpec.ToSlug(frontmatter.Name); + if (!SkillFrontmatterSpec.ValidateName(slug, out var slugReason)) + { + var badSlugReason = $"Derived slug '{slug}' from name '{frontmatter.Name}' is invalid: {slugReason}"; + logger.LogWarning( + "Skill curation failed — session={Session} reason={Reason}", + checkpoint.SessionId, badSlugReason); + var failed = new SkillCurationResult( + SkillCurationOutcome.Failed, + FailureReason: badSlugReason, + TurnsDigested: digestTurns, + Model: modelId); + await AppendCurationLogAsync(checkpoint.SessionId, failed, source, ct); + return failed; + } + + // Guarantee the written file's 'name:' matches the directory it's written under — the + // LLM's raw name may need slugifying (spaces, uppercase, ...), and without this the file + // and its own directory would disagree, which fuseraft's orchestration skills provider + // treats as invalid and silently drops. + skillContent = SkillFrontmatterSpec.WithCanonicalName(skillContent, slug); try { @@ -410,9 +442,6 @@ private async Task AppendCurationLogAsync( } } - private static string ToSlug(string name) => - Regex.Replace(name.ToLowerInvariant().Trim(), @"[^a-z0-9]+", "-").Trim('-'); - private sealed record CurationLogEntry( string Ts, string Session, diff --git a/src/Program.cs b/src/Program.cs index e9f1a70f..ccd7020c 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -144,6 +144,7 @@ services.AddTransient(); services.AddTransient(); services.AddTransient(); +services.AddTransient(); services.AddTransient(); services.AddTransient(); services.AddTransient(); @@ -331,6 +332,11 @@ .WithExample(["skills", "curation-log"]) .WithExample(["skills", "curation-log", "--last", "20"]) .WithExample(["skills", "curation-log", "--outcome", "failed"]); + + branch.AddCommand("validate") + .WithDescription("Validate a SKILL.md's frontmatter against the Agent Skills specification.") + .WithExample(["skills", "validate"]) + .WithExample(["skills", "validate", "../skills/sandbox-test"]); }); cfg.AddBranch("log", branch => diff --git a/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs b/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs index 5c9893a8..cd78eefb 100644 --- a/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs +++ b/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs @@ -325,4 +325,135 @@ public void BuildSkills_SkillMdIsDirectory_DoesNotThrow() var ex = Record.Exception(() => ReplSkillsLoader.BuildSkills([_root])); Assert.Null(ex); } + + // ── BuildSkillsDetailed — spec conformance (parity with orchestration) ──── + + [Fact] + public void BuildSkillsDetailed_NameDoesNotMatchDirectory_SkipsWithWarning() + { + WriteSkill("mismatched-dir", ValidSkillMd("totally-different-name", "A description.")); + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.Null(result.Plugin); + Assert.Contains(result.Warnings, w => w.Contains("does not match") && w.Contains("mismatched-dir")); + } + + [Fact] + public void BuildSkillsDetailed_DeclaredNameInvalidFormat_SkipsWithWarning() + { + WriteSkill("Bad-Name", ValidSkillMd("Bad-Name", "A description.")); // uppercase not allowed + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.Null(result.Plugin); + Assert.Single(result.Warnings); + } + + [Fact] + public void BuildSkillsDetailed_DeclaredDescriptionTooLong_SkipsWithWarning() + { + var longDescription = new string('a', 1025); + WriteSkill("my-skill", ValidSkillMd("my-skill", longDescription)); + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.Null(result.Plugin); + Assert.Contains(result.Warnings, w => w.Contains("1024")); + } + + [Fact] + public void BuildSkillsDetailed_NoNameFieldAtAll_LoadsLeniently_NoWarning() + { + // Directory-name-only skills (no 'name:' field) remain a supported, warning-free + // REPL convenience even though orchestration requires a declared, matching name. + WriteSkill("my-skill", "---\ndescription: \"A description.\"\n---\n\nBody."); + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.NotNull(result.Plugin); + Assert.Equal(1, result.Plugin!.Count); + Assert.Empty(result.Warnings); + } + + [Fact] + public void BuildSkillsDetailed_ValidNameAndDirectoryMatch_NoWarning() + { + WriteSkill("my-skill", ValidSkillMd("my-skill", "A description.")); + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.NotNull(result.Plugin); + Assert.Empty(result.Warnings); + } + + [Fact] + public void BuildSkills_CompatibilityField_AppearsInCatalog() + { + var dir = Path.Combine(_root, "my-skill"); + Directory.CreateDirectory(dir); + File.WriteAllText(Path.Combine(dir, "SKILL.md"), + "---\nname: my-skill\ndescription: \"A description.\"\ncompatibility: \"Requires docker\"\n---\n\nBody."); + + var (_, catalog) = ReplSkillsLoader.BuildSkills([_root]); + + Assert.Contains("Requires docker", catalog!); + } + + [Fact] + public void BuildSkillsDetailed_DeclaredCompatibilityTooLong_SkipsWithWarning() + { + var dir = Path.Combine(_root, "my-skill"); + Directory.CreateDirectory(dir); + var longCompat = new string('a', 501); + File.WriteAllText(Path.Combine(dir, "SKILL.md"), + $"---\nname: my-skill\ndescription: \"A description.\"\ncompatibility: \"{longCompat}\"\n---\n\nBody."); + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.Null(result.Plugin); + Assert.Contains(result.Warnings, w => w.Contains("500")); + } + + [Fact] + public void BuildSkillsDetailed_NestedVendorNamespace_TwoLevelsDeep_IsDiscovered() + { + // Matches orchestration's AgentFileSkillsSource search depth (root/vendor/skill/SKILL.md). + WriteSkill(Path.Combine("vendor", "my-skill"), ValidSkillMd("my-skill", "A description.")); + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.NotNull(result.Plugin); + Assert.True(result.Plugin!.HasSkill("my-skill")); + } + + [Fact] + public void BuildSkillsDetailed_SymlinkedSkillDirectory_IsNotFollowed() + { + var realSkillRoot = Path.Combine(Path.GetTempPath(), "fuseraft_loader_tests_real_" + Guid.NewGuid().ToString("N")[..8]); + var realSkillDir = Path.Combine(realSkillRoot, "real-skill"); + Directory.CreateDirectory(realSkillDir); + File.WriteAllText(Path.Combine(realSkillDir, "SKILL.md"), ValidSkillMd("real-skill", "A description.")); + + try + { + var link = Path.Combine(_root, "linked-skill"); + try + { + Directory.CreateSymbolicLink(link, realSkillDir); + } + catch (Exception) + { + return; // environment doesn't allow symlinks — skip + } + + var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + + Assert.Null(result.Plugin); // the only skill lives behind a symlink, which must not be followed + } + finally + { + Directory.Delete(realSkillRoot, recursive: true); + } + } } diff --git a/tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs b/tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs new file mode 100644 index 00000000..b00b074e --- /dev/null +++ b/tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs @@ -0,0 +1,274 @@ +using fuseraft.Core.Skills; + +namespace FuseraftCli.Tests; + +/// +/// Tests for — the single parser/validator shared by the +/// REPL loader, orchestration-visible skills add/skills validate commands, and +/// skill curation, so all surfaces agree on what a spec-conformant SKILL.md looks like. +/// +public sealed class SkillFrontmatterSpecTests +{ + // ── TryParse ───────────────────────────────────────────────────────────── + + [Fact] + public void TryParse_NoFrontmatter_ReturnsNull() + { + Assert.Null(SkillFrontmatterSpec.TryParse("# Just a heading\n\nSome body text.")); + } + + [Fact] + public void TryParse_EmptyContent_ReturnsNull() + { + Assert.Null(SkillFrontmatterSpec.TryParse("")); + Assert.Null(SkillFrontmatterSpec.TryParse(null)); + } + + [Fact] + public void TryParse_UnclosedFrontmatter_ReturnsNull() + { + Assert.Null(SkillFrontmatterSpec.TryParse("---\nname: skill\n\nNo closing delimiter")); + } + + [Fact] + public void TryParse_MinimalValidFrontmatter_ExtractsNameAndDescription() + { + var fm = SkillFrontmatterSpec.TryParse("---\nname: pdf-processing\ndescription: Extract PDF text.\n---\n\nBody"); + Assert.NotNull(fm); + Assert.Equal("pdf-processing", fm!.Name); + Assert.Equal("Extract PDF text.", fm.Description); + } + + [Fact] + public void TryParse_DoubleQuotedDescriptionWithColon_PreservesColon() + { + var fm = SkillFrontmatterSpec.TryParse("---\ndescription: \"Use when: A, B, or C.\"\n---"); + Assert.Equal("Use when: A, B, or C.", fm!.Description); + } + + [Fact] + public void TryParse_SingleQuotedValue_Unquotes() + { + var fm = SkillFrontmatterSpec.TryParse("---\ndescription: 'Use when doing Y.'\n---"); + Assert.Equal("Use when doing Y.", fm!.Description); + } + + [Fact] + public void TryParse_OptionalFields_AllExtracted() + { + var content = """ + --- + name: pdf-processing + description: Extract PDF text, fill forms, merge files. + license: Apache-2.0 + compatibility: Requires Python 3.14+ and uv + allowed-tools: Bash(git:*) Bash(jq:*) Read + metadata: + author: example-org + version: "1.0" + --- + + Body. + """; + + var fm = SkillFrontmatterSpec.TryParse(content); + + Assert.NotNull(fm); + Assert.Equal("Apache-2.0", fm!.License); + Assert.Equal("Requires Python 3.14+ and uv", fm.Compatibility); + Assert.Equal("Bash(git:*) Bash(jq:*) Read", fm.AllowedTools); + Assert.NotNull(fm.Metadata); + Assert.Equal("example-org", fm.Metadata!["author"]); + Assert.Equal("1.0", fm.Metadata["version"]); + } + + [Fact] + public void TryParse_MetadataKeysDoNotLeakIntoTopLevelFields() + { + // A "name:" or "description:" line indented under metadata: must not be picked up + // as the top-level field — only unindented lines are top-level. + var content = """ + --- + name: real-skill + description: Real description. + metadata: + name: this-is-just-metadata + --- + """; + + var fm = SkillFrontmatterSpec.TryParse(content); + + Assert.Equal("real-skill", fm!.Name); + Assert.Equal("this-is-just-metadata", fm.Metadata!["name"]); + } + + [Fact] + public void TryParse_UnrecognizedTopLevelKeys_AreIgnoredWithoutError() + { + // Third-party skills sometimes add non-spec fields (e.g. "version:", "homepage:"). + var content = "---\nname: my-skill\ndescription: A skill.\nversion: 1.5.2\nhomepage: https://example.com\n---"; + var fm = SkillFrontmatterSpec.TryParse(content); + + Assert.Equal("my-skill", fm!.Name); + Assert.Equal("A skill.", fm.Description); + } + + [Fact] + public void TryParse_NoRecognizedFields_ReturnsNull() + { + Assert.Null(SkillFrontmatterSpec.TryParse("---\n: invalid yaml :\n---")); + } + + // ── ValidateName ───────────────────────────────────────────────────────── + + [Theory] + [InlineData("pdf-processing")] + [InlineData("data-analysis")] + [InlineData("a")] + [InlineData("a1-b2")] + public void ValidateName_ValidNames_Pass(string name) + { + Assert.True(SkillFrontmatterSpec.ValidateName(name, out var reason)); + Assert.Null(reason); + } + + [Fact] + public void ValidateName_Null_Fails() + { + Assert.False(SkillFrontmatterSpec.ValidateName(null, out var reason)); + Assert.Contains("required", reason); + } + + [Fact] + public void ValidateName_TooLong_Fails() + { + var name = new string('a', 65); + Assert.False(SkillFrontmatterSpec.ValidateName(name, out var reason)); + Assert.Contains("64", reason); + } + + [Theory] + [InlineData("PDF-Processing")] + [InlineData("-pdf")] + [InlineData("pdf-")] + [InlineData("pdf--processing")] + [InlineData("pdf_processing")] + [InlineData("pdf processing")] + public void ValidateName_InvalidFormats_Fail(string name) + { + Assert.False(SkillFrontmatterSpec.ValidateName(name, out var reason)); + Assert.NotNull(reason); + } + + // ── ValidateDescription ────────────────────────────────────────────────── + + [Fact] + public void ValidateDescription_Empty_Fails() + { + Assert.False(SkillFrontmatterSpec.ValidateDescription("", out var reason)); + Assert.Contains("required", reason); + } + + [Fact] + public void ValidateDescription_TooLong_Fails() + { + var desc = new string('a', 1025); + Assert.False(SkillFrontmatterSpec.ValidateDescription(desc, out var reason)); + Assert.Contains("1024", reason); + } + + [Fact] + public void ValidateDescription_ExactlyMaxLength_Passes() + { + var desc = new string('a', 1024); + Assert.True(SkillFrontmatterSpec.ValidateDescription(desc, out _)); + } + + // ── ValidateCompatibility ──────────────────────────────────────────────── + + [Fact] + public void ValidateCompatibility_Null_Passes() + { + Assert.True(SkillFrontmatterSpec.ValidateCompatibility(null, out var reason)); + Assert.Null(reason); + } + + [Fact] + public void ValidateCompatibility_TooLong_Fails() + { + Assert.False(SkillFrontmatterSpec.ValidateCompatibility(new string('a', 501), out var reason)); + Assert.Contains("500", reason); + } + + // ── Validate (full conformance) ────────────────────────────────────────── + + [Fact] + public void Validate_NullFrontmatter_ReportsMissingFrontmatter() + { + var violations = SkillFrontmatterSpec.Validate(null, "my-skill"); + Assert.Single(violations); + Assert.Contains("frontmatter", violations[0]); + } + + [Fact] + public void Validate_NameDoesNotMatchDirectory_ReportsMismatch() + { + var fm = new SkillFrontmatter("my-skill", "A description.", null, null, null, null); + var violations = SkillFrontmatterSpec.Validate(fm, "different-dir"); + Assert.Contains(violations, v => v.Contains("does not match")); + } + + [Fact] + public void Validate_FullyCompliant_ReturnsNoViolations() + { + var fm = new SkillFrontmatter("my-skill", "A description.", "MIT", "Requires docker", null, null); + var violations = SkillFrontmatterSpec.Validate(fm, "my-skill"); + Assert.Empty(violations); + } + + // ── ToSlug ─────────────────────────────────────────────────────────────── + + [Theory] + [InlineData("PDF Processing", "pdf-processing")] + [InlineData("My Bad Skill!!", "my-bad-skill")] + [InlineData(" leading and trailing ", "leading-and-trailing")] + [InlineData("already-a-slug", "already-a-slug")] + public void ToSlug_ProducesValidSlug(string input, string expected) + { + var slug = SkillFrontmatterSpec.ToSlug(input); + Assert.Equal(expected, slug); + Assert.True(SkillFrontmatterSpec.ValidateName(slug, out _)); + } + + // ── WithCanonicalName ──────────────────────────────────────────────────── + + [Fact] + public void WithCanonicalName_ReplacesExistingNameField() + { + var content = "---\nname: My Bad Skill!!\ndescription: A description.\n---\n\nBody"; + var rewritten = SkillFrontmatterSpec.WithCanonicalName(content, "my-bad-skill"); + + var fm = SkillFrontmatterSpec.TryParse(rewritten); + Assert.Equal("my-bad-skill", fm!.Name); + Assert.Equal("A description.", fm.Description); // untouched + Assert.Contains("Body", rewritten); // body untouched + } + + [Fact] + public void WithCanonicalName_NoExistingNameField_InsertsOne() + { + var content = "---\ndescription: A description.\n---\n\nBody"; + var rewritten = SkillFrontmatterSpec.WithCanonicalName(content, "new-slug"); + + var fm = SkillFrontmatterSpec.TryParse(rewritten); + Assert.Equal("new-slug", fm!.Name); + Assert.Equal("A description.", fm.Description); + } + + [Fact] + public void WithCanonicalName_NoFrontmatter_ReturnsContentUnchanged() + { + const string content = "# No frontmatter\n\nJust body text."; + Assert.Equal(content, SkillFrontmatterSpec.WithCanonicalName(content, "some-slug")); + } +} diff --git a/tests/FuseraftCli.Tests/SkillPathGuardTests.cs b/tests/FuseraftCli.Tests/SkillPathGuardTests.cs new file mode 100644 index 00000000..5669101c --- /dev/null +++ b/tests/FuseraftCli.Tests/SkillPathGuardTests.cs @@ -0,0 +1,155 @@ +using fuseraft.Core.Skills; + +namespace FuseraftCli.Tests; + +/// +/// Tests for — path-containment and symlink-escape checks used by +/// SkillsPlugin (read_skill_resource/run_skill_script) before touching a +/// path the model supplied. +/// +public sealed class SkillPathGuardTests : IDisposable +{ + private readonly string _root; + private readonly string _skillDir; + private readonly string _outsideDir; + + public SkillPathGuardTests() + { + _root = Path.Combine(Path.GetTempPath(), "fuseraft_pathguard_tests_" + Guid.NewGuid().ToString("N")[..8]); + _skillDir = Path.Combine(_root, "my-skill"); + _outsideDir = Path.Combine(_root, "outside"); + Directory.CreateDirectory(_skillDir); + Directory.CreateDirectory(_outsideDir); + } + + public void Dispose() => Directory.Delete(_root, recursive: true); + + /// True on platforms/environments where creating symlinks is permitted (skips on restricted CI/sandboxes). + private bool CanCreateSymlinks(out string reason) + { + try + { + var link = Path.Combine(_root, "probe-link-" + Guid.NewGuid().ToString("N")[..6]); + File.CreateSymbolicLink(link, Path.Combine(_root, "probe-target")); + File.Delete(link); + reason = ""; + return true; + } + catch (Exception ex) + { + reason = ex.Message; + return false; + } + } + + [Fact] + public void TryResolveSafePath_PlainNestedFile_Succeeds() + { + Directory.CreateDirectory(Path.Combine(_skillDir, "references")); + File.WriteAllText(Path.Combine(_skillDir, "references", "guide.md"), "content"); + + var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "references/guide.md", out var fullPath, out var reason); + + Assert.True(ok); + Assert.Null(reason); + Assert.EndsWith(Path.Combine("references", "guide.md"), fullPath); + } + + [Theory] + [InlineData("../secret.txt")] + [InlineData("references/../../secret.txt")] + public void TryResolveSafePath_TraversalOutsideRoot_Fails(string relative) + { + File.WriteAllText(Path.Combine(_root, "secret.txt"), "top secret"); + + var ok = SkillPathGuard.TryResolveSafePath(_skillDir, relative, out _, out var reason); + + Assert.False(ok); + Assert.Contains("outside", reason); + } + + [Fact] + public void TryResolveSafePath_AbsolutePathEscape_Fails() + { + var outsideFile = Path.Combine(_outsideDir, "secret.txt"); + File.WriteAllText(outsideFile, "top secret"); + + var ok = SkillPathGuard.TryResolveSafePath(_skillDir, outsideFile, out _, out var reason); + + Assert.False(ok); + Assert.Contains("outside", reason); + } + + [Fact] + public void TryResolveSafePath_SymlinkedFilePointingOutside_Fails() + { + if (!CanCreateSymlinks(out _)) return; // environment doesn't allow symlinks — skip + + var secret = Path.Combine(_outsideDir, "secret.txt"); + File.WriteAllText(secret, "top secret"); + var link = Path.Combine(_skillDir, "innocuous.md"); + File.CreateSymbolicLink(link, secret); + + var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "innocuous.md", out _, out var reason); + + Assert.False(ok); + Assert.Contains("symlink", reason); + } + + [Fact] + public void TryResolveSafePath_SymlinkedSubdirectoryPointingOutside_Fails() + { + if (!CanCreateSymlinks(out _)) return; + + var secretDir = Path.Combine(_outsideDir, "secret-dir"); + Directory.CreateDirectory(secretDir); + File.WriteAllText(Path.Combine(secretDir, "file.txt"), "top secret"); + + var linkedDir = Path.Combine(_skillDir, "references"); + Directory.CreateSymbolicLink(linkedDir, secretDir); + + var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "references/file.txt", out _, out var reason); + + Assert.False(ok); + Assert.Contains("symlink", reason); + } + + [Fact] + public void TryResolveSafePath_NonExistentPath_StillReportsContainment() + { + // A not-yet-existing path inside the root should pass the guard; the caller's own + // File.Exists check is responsible for reporting "not found". + var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "references/missing.md", out _, out var reason); + + Assert.True(ok); + Assert.Null(reason); + } + + [Fact] + public void IsReparsePoint_RegularFile_ReturnsFalse() + { + var file = Path.Combine(_skillDir, "plain.txt"); + File.WriteAllText(file, "content"); + + Assert.False(SkillPathGuard.IsReparsePoint(file)); + } + + [Fact] + public void IsReparsePoint_NonExistentPath_ReturnsFalse() + { + Assert.False(SkillPathGuard.IsReparsePoint(Path.Combine(_skillDir, "missing"))); + } + + [Fact] + public void IsReparsePoint_Symlink_ReturnsTrue() + { + if (!CanCreateSymlinks(out _)) return; + + var target = Path.Combine(_outsideDir, "target.txt"); + File.WriteAllText(target, "content"); + var link = Path.Combine(_skillDir, "link.txt"); + File.CreateSymbolicLink(link, target); + + Assert.True(SkillPathGuard.IsReparsePoint(link)); + } +} diff --git a/tests/FuseraftCli.Tests/SkillsHelpersTests.cs b/tests/FuseraftCli.Tests/SkillsHelpersTests.cs index af2a6631..540187ba 100644 --- a/tests/FuseraftCli.Tests/SkillsHelpersTests.cs +++ b/tests/FuseraftCli.Tests/SkillsHelpersTests.cs @@ -94,4 +94,37 @@ public void CopySkillDirectory_CreatesDestDirectory_WhenMissing() Assert.True(Directory.Exists(_destDir)); } + + // ── ExtractSlug / ExtractDescription / CanonicalizeName ──────────────────── + + [Fact] + public void ExtractSlug_SlugifiesRawName() + { + var content = "---\nname: My Bad Skill!!\ndescription: A skill.\n---"; + Assert.Equal("my-bad-skill", SkillsHelpers.ExtractSlug(content)); + } + + [Fact] + public void ExtractSlug_NoNameField_ReturnsNull() + { + Assert.Null(SkillsHelpers.ExtractSlug("---\ndescription: A skill.\n---")); + } + + [Fact] + public void CanonicalizeName_NameAlreadyMatchesSlug_ReturnsContentUnchanged() + { + const string content = "---\nname: my-skill\ndescription: A skill.\n---\n\nBody"; + Assert.Same(content, SkillsHelpers.CanonicalizeName(content, "my-skill")); + } + + [Fact] + public void CanonicalizeName_NameDiffersFromSlug_RewritesNameField() + { + var content = "---\nname: My Bad Skill!!\ndescription: A skill.\n---\n\nBody"; + var rewritten = SkillsHelpers.CanonicalizeName(content, "my-bad-skill"); + + Assert.Equal("my-bad-skill", SkillsHelpers.ExtractSlug(rewritten)); + Assert.Equal("A skill.", SkillsHelpers.ExtractDescription(rewritten)); + Assert.Contains("Body", rewritten); + } } diff --git a/tests/FuseraftCli.Tests/SkillsPluginTests.cs b/tests/FuseraftCli.Tests/SkillsPluginTests.cs index dd8e24b2..5a72aef0 100644 --- a/tests/FuseraftCli.Tests/SkillsPluginTests.cs +++ b/tests/FuseraftCli.Tests/SkillsPluginTests.cs @@ -172,6 +172,31 @@ public async Task ReadSkillResource_AbsolutePathEscape_ReturnsError() Assert.DoesNotContain("top secret", result); } + [Fact] + public async Task ReadSkillResource_SymlinkedFilePointingOutside_ReturnsError() + { + var dir = MakeSkillDir("my-skill", "body"); + var secret = Path.Combine(_root, "secret.txt"); + File.WriteAllText(secret, "top secret"); + + string link; + try + { + link = Path.Combine(dir, "innocuous.md"); + File.CreateSymbolicLink(link, secret); + } + catch (Exception) + { + return; // environment doesn't allow symlinks — skip + } + + var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); + var result = await plugin.ReadSkillResourceAsync("my-skill", "innocuous.md"); + + Assert.StartsWith("[ERROR]", result); + Assert.DoesNotContain("top secret", result); + } + // ── RunSkillScriptAsync ─────────────────────────────────────────────────── [Fact] @@ -195,6 +220,31 @@ public async Task RunSkillScript_PathTraversal_ReturnsError() Assert.DoesNotContain("pwned", result); } + [Fact] + public async Task RunSkillScript_SymlinkedScriptPointingOutside_ReturnsError() + { + var dir = MakeSkillDir("my-skill", "body"); + var outsideScript = Path.Combine(_root, "evil.sh"); + File.WriteAllText(outsideScript, "#!/bin/sh\necho pwned\n"); + + string link; + try + { + link = Path.Combine(dir, "run.sh"); + File.CreateSymbolicLink(link, outsideScript); + } + catch (Exception) + { + return; // environment doesn't allow symlinks — skip + } + + var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); + var result = await plugin.RunSkillScriptAsync("my-skill", "run.sh"); + + Assert.StartsWith("[ERROR]", result); + Assert.DoesNotContain("pwned", result); + } + [Fact] public async Task RunSkillScript_NestedScriptPath_Runs() { From 6ece9809ae3fea71903b68b79789de002b6c0b71 Mon Sep 17 00:00:00 2001 From: Scott Stauffer Date: Fri, 28 Aug 2026 22:43:20 -0500 Subject: [PATCH 2/2] feat(skills): route skill loading through Microsoft Agent Framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the hand-rolled SkillFrontmatterSpec/SkillPathGuard/SkillsPlugin (added earlier the same day) with direct use of Microsoft.Agents.AI's AgentFileSkillsSource/AgentSkillsProvider for both the REPL and orchestration — one implementation instead of two that happened to agree, per user direction to use out-of-the-box tools rather than reimplementing spec parsing/validation. ReplSkillsLoader is now a thin wrapper that wraps the REPL's IChatClient in a throwaway ChatClientAgent to satisfy the framework's AIAgent context requirement; SkillsPlugin.cs is gone entirely, replaced by AgentSkillsProvider's own load_skill/read_skill_resource/ run_skill_script tools. While wiring this up, found and fixed a real, pre-existing bug: AgentSkillsProvider wraps its tools in ApprovalRequiredAIFunction by default, which only resolves through Microsoft's ToolApprovalAgentOptions pipeline — fuseraft has no wiring for that anywhere, so orchestration's skill tools were silently non-functional (confirmed live: a real `fuseraft run` session asking the model to call load_skill returned empty text and 0 tool calls). Fixed by disabling approval for all three tools, since an unresolved gate is strictly worse than none until real approval wiring exists. `skills add` keeps its lenient auto-canonicalization by explicit user decision (it derives an install slug from a raw title and rewrites the installed name: field to match); `skills validate`, `skills list`, and SkillCurator are now strict, deferring entirely to AgentSkillFrontmatter's own constructor/static validators. The only hand-written parsing left is FrontmatterFieldReader, a ~50-line read-only "grab one YAML field's raw value" utility needed only to bootstrap where to place a file before Microsoft's API — which requires a correctly-named directory — can validate it at all. --- docs/security.md | 2 +- docs/skills.md | 8 +- skills/skill-author/SKILL.md | 6 +- src/Cli/Commands/Repl/ReplCommand.cs | 38 +- src/Cli/Commands/Repl/ReplSessionContext.cs | 3 +- src/Cli/Commands/Repl/ReplSkillsLoader.cs | 301 +++--------- src/Cli/Commands/Repl/ReplTurn.cs | 9 +- src/Cli/Commands/Skills/SkillsAddCommand.cs | 29 +- src/Cli/Commands/Skills/SkillsHelpers.cs | 61 ++- src/Cli/Commands/Skills/SkillsListCommand.cs | 40 +- .../Commands/Skills/SkillsValidateCommand.cs | 75 ++- src/Cli/OrchestratorBuilder.cs | 70 +-- src/Core/Skills/FrontmatterFieldReader.cs | 61 +++ src/Core/Skills/FuseraftSkillsSources.cs | 158 ++++++ src/Core/Skills/SkillDiscoveryAgent.cs | 38 ++ src/Core/Skills/SkillFrontmatter.cs | 266 ----------- src/Core/Skills/SkillPathGuard.cs | 71 --- src/Infrastructure/Plugins/SkillsPlugin.cs | 148 ------ src/Orchestration/Skills/SkillCurator.cs | 57 +-- .../ReplSkillsLoaderTests.cs | 451 ++++-------------- .../SkillFrontmatterSpecTests.cs | 274 ----------- .../FuseraftCli.Tests/SkillPathGuardTests.cs | 155 ------ tests/FuseraftCli.Tests/SkillsHelpersTests.cs | 3 +- tests/FuseraftCli.Tests/SkillsPluginTests.cs | 315 ------------ 24 files changed, 607 insertions(+), 2032 deletions(-) create mode 100644 src/Core/Skills/FrontmatterFieldReader.cs create mode 100644 src/Core/Skills/FuseraftSkillsSources.cs create mode 100644 src/Core/Skills/SkillDiscoveryAgent.cs delete mode 100644 src/Core/Skills/SkillFrontmatter.cs delete mode 100644 src/Core/Skills/SkillPathGuard.cs delete mode 100644 src/Infrastructure/Plugins/SkillsPlugin.cs delete mode 100644 tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs delete mode 100644 tests/FuseraftCli.Tests/SkillPathGuardTests.cs delete mode 100644 tests/FuseraftCli.Tests/SkillsPluginTests.cs diff --git a/docs/security.md b/docs/security.md index 0ddcf6df..9de41123 100644 --- a/docs/security.md +++ b/docs/security.md @@ -434,7 +434,7 @@ If `fuseraft run --work-dir` points at a directory you did not author, any skill - Only run `fuseraft` in working directories you trust. Treat `.agents/skills/` and `.fuseraft/skills/` in a cloned repo the same way you would treat a `Makefile` or `package.json` postinstall script. - For higher assurance, run fuseraft inside a Docker container (`CodeExecution` plugin) where the host environment is not exposed. -- `UseScriptApproval` support is planned — when enabled it will require explicit user confirmation before any skill script executes. Until then, script execution is automatic once a skill is loaded. +- Microsoft Agent Framework's skills provider supports gating `load_skill`/`read_skill_resource`/`run_skill_script` behind an approval step (`AgentSkillsProviderOptions`), but fuseraft explicitly disables it today, since neither the REPL nor orchestration has a pipeline that resolves an approval request — leaving it enabled would make the tools non-functional rather than gated. Script execution is therefore automatic once a skill is loaded; wiring real approval (REPL: a confirmation prompt; orchestration: `IHumanApprovalService`) is a known future improvement, not yet implemented. - `read_skill_resource` and `run_skill_script` resolve the model-supplied path against the skill directory and reject anything that resolves outside it, including via a symlinked file or subdirectory planted inside the skill folder — this narrows path-based escape from *within* a loaded skill, but a fully malicious skill script still runs as an OS subprocess with the full process environment; it isn't a substitute for only loading trusted skills. --- diff --git a/docs/skills.md b/docs/skills.md index 7cd64db8..a5eb639f 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -1,6 +1,6 @@ # Skills -Skills give agents specialized knowledge and step-by-step procedures for specific types of tasks. At REPL startup fuseraft scans your skill directories, injects a catalog of available skills into the system prompt, and exposes two tools the model can call to use them. +Skills give agents specialized knowledge and step-by-step procedures for specific types of tasks, following the [Agent Skills specification](https://agentskills.io/specification). At session start fuseraft scans your skill directories, injects a catalog of available skills into the system prompt, and exposes tools the model can call to use them. Discovery, frontmatter parsing/validation, and the skill tools themselves all come from the [Microsoft Agent Framework](https://github.com/microsoft/agent-framework)'s `AgentFileSkillsSource`/`AgentSkillsProvider` — the REPL and `fuseraft run` orchestration sessions share the exact same implementation, so a skill is treated identically in both. --- @@ -40,7 +40,7 @@ At startup, the skill count appears in the compact info line alongside the activ If `--no-tools` is passed, skills are disabled for that session. -`fuseraft run` orchestration sessions use the same five discovery locations and the same three tools (`load_skill`, `read_skill_resource`, `run_skill_script`), wired onto every agent automatically whenever at least one skill directory exists — there is no need to add `Skills` to an agent's `Plugins:` list, though doing so as a declaration of intent is harmless. +`fuseraft run` orchestration sessions use the same five discovery locations and the same three tools (`load_skill`, `read_skill_resource`, `run_skill_script`), wired onto every agent automatically whenever at least one skill directory exists — there is no need to add `Skills` to an agent's `Plugins:` list, though doing so as a declaration of intent is harmless. This is the same discovery pipeline the REPL uses, not a separate implementation — a skill either works identically in both, or (if its frontmatter is invalid) in neither. --- @@ -220,9 +220,9 @@ fuseraft follows the [Agent Skills specification](https://agentskills.io/specifi If your instructions are long, move reference material into a `references/` subdirectory inside the skill folder. The agent loads those files on demand — with `read_skill_resource` — rather than all at once. `scripts/` and `assets/` are supported the same way. -**If two installed skills share the same name**, the one in the higher-precedence location wins and a warning is logged. +**If two installed skills share the same name**, the one in the higher-precedence location wins. -> **Keep `name:` and the directory name identical.** `fuseraft run` orchestration sessions require `name:` to match the directory name **exactly** (case-sensitive), to be valid lowercase kebab-case, and require a non-empty, correctly-sized `description:`. A skill that violates any of these is silently dropped from the orchestration catalog. The REPL's loader is more lenient — a `SKILL.md` with no frontmatter at all still loads, using the directory name as its slug — but the moment a `name:`, `description:`, or `compatibility:` field *is* declared, the REPL validates it against the same rules and **skips the skill with a warning** on a violation (most commonly a name/directory mismatch), rather than silently loading something that would vanish under `fuseraft run`. Run `fuseraft skills validate [path]` to check a skill (or every installed skill) against the full specification before relying on it. +> **`name:` must match the directory name exactly.** Both the REPL and `fuseraft run` require `name:` to match its parent directory name **exactly** (case-sensitive), to be valid lowercase kebab-case, and require a non-empty, correctly-sized `description:` — they use the identical discovery pipeline, so there is no REPL-specific leniency here. A skill that violates any of these is silently excluded from the catalog in **both** surfaces, with the reason logged as a warning or error (visible by default — no `--verbose` needed). Run `fuseraft skills validate [path]` to check a skill (or every installed skill) against the full specification before relying on it. The one exception is `fuseraft skills add`, which stays deliberately lenient — see [Installing skills](#for-all-your-projects) above. --- diff --git a/skills/skill-author/SKILL.md b/skills/skill-author/SKILL.md index 87974517..5109497a 100644 --- a/skills/skill-author/SKILL.md +++ b/skills/skill-author/SKILL.md @@ -44,7 +44,7 @@ description: --- ``` -**`name`:** A short, lowercase kebab-case slug (e.g. `debug-session`, `mcp-setup`) — letters, digits, and single hyphens only, no leading/trailing/double hyphens, max 64 characters. This is used as the install directory name when running `fuseraft skills add`. Keep it to 1–3 words, and **make it identical to the skill's directory name**: `fuseraft run` orchestration sessions silently drop the skill from the catalog if `name:` doesn't exactly match the directory name (or isn't valid kebab-case, or `description:` is empty or too long). The REPL loader is more lenient about a skill with no frontmatter at all, but once `name:` is present it applies the same check and skips the skill (with a warning) on a mismatch. Matching them keeps the skill working identically in both surfaces — run `fuseraft skills validate ` to confirm before installing. +**`name`:** A short, lowercase kebab-case slug (e.g. `debug-session`, `mcp-setup`) — letters, digits, and single hyphens only, no leading/trailing/double hyphens, max 64 characters. This is used as the install directory name when running `fuseraft skills add`. Keep it to 1–3 words, and **make it identical to the skill's directory name**: the REPL and `fuseraft run` orchestration sessions both use the same discovery pipeline and silently exclude the skill from the catalog if `name:` doesn't exactly match the directory name (or isn't valid kebab-case, or `description:` is empty or too long) — there is no REPL-specific leniency once a skill directory exists somewhere fuseraft scans. Run `fuseraft skills validate ` to confirm before installing. **Optional fields**, per the [Agent Skills specification](https://agentskills.io/specification) — add only when they earn their keep: - **`license`:** a license name or reference to a bundled license file. Only relevant for skills you intend to share/distribute. @@ -176,7 +176,7 @@ Or write directly to `~/.fuseraft/skills//SKILL.md` — fuseraft loads fro First, run `fuseraft skills validate ` (or `fuseraft skills validate` with no argument once installed, to check it alongside every other installed skill). This checks the frontmatter against the full specification — name format and directory match, description presence/length, compatibility length — with the same validator both the REPL and orchestration use, before you burn a session on it. -For **REPL sessions**, start or restart fuseraft and run `/tools`. The skill should appear under the `Skills` category with its name and description. Watch the startup output for a `⚠ Skipped skill at ...` warning — that means the frontmatter is present but invalid, and the skill did not load. +For **REPL sessions**, start or restart fuseraft and run `/tools`. The skill should appear under the `Skills` category with its name and description. Watch the startup output for an `[ERR]`/`[WRN]` line naming the SKILL.md path — that means the frontmatter is invalid (most often a name/directory mismatch) and the skill did not load. For **orchestration sessions**, run `fuseraft validate` on the config first, then do a one-turn dry run: @@ -187,7 +187,7 @@ fuseraft run --config --max-iterations 1 "List your available skills." The agent should name the skill in its response. If it does not appear, check: - `SKILL.md` is directly inside the skill directory (not nested deeper) - The install path is one of the five recognized locations (project `.fuseraft/skills/`, project `.agents/skills/`, user `.fuseraft/skills/`, user `.agents/skills/`, or shipped built-in) -- `fuseraft skills validate` passes — a violation it reports is silently dropped by `fuseraft run`'s stricter loader with no error to the user, only a log entry +- `fuseraft skills validate` passes — a violation it reports means the skill is silently excluded from both the REPL and `fuseraft run` catalogs, with no error to the user beyond a log entry ### Step 8: Refine the Description diff --git a/src/Cli/Commands/Repl/ReplCommand.cs b/src/Cli/Commands/Repl/ReplCommand.cs index b5b2af14..78d3d0d4 100644 --- a/src/Cli/Commands/Repl/ReplCommand.cs +++ b/src/Cli/Commands/Repl/ReplCommand.cs @@ -1,4 +1,5 @@ using System.ComponentModel; +using Microsoft.Agents.AI; using Microsoft.Extensions.AI; using Microsoft.Extensions.Logging; using Spectre.Console; @@ -182,7 +183,7 @@ protected override async Task ExecuteAsync( var toolsByCategory = new Dictionary>(StringComparer.OrdinalIgnoreCase); using ShellPlugin? shellPlugin = settings.NoTools ? null : new ShellPlugin(shellPolicy: TryLoadDefaultShellPolicy()); SubAgentPlugin? subAgent = null; - SkillsPlugin? skillsPlugin = null; + IReadOnlyList discoveredSkills = []; string? skillsCatalog = null; List? explorerTools = null; TodoPlugin? todoPlugin = null; @@ -220,14 +221,6 @@ protected override async Task ExecuteAsync( toolsByCategory["FileSystem"] = fsFunctions.Where(f => CoreFileSystemTools.Contains(f.Name)).ToList(); toolsByCategory["Shell"] = shellFunctions.Where(f => CoreShellTools.Contains(f.Name)).ToList(); toolsByCategory["Git"] = gitFunctions.Where(f => CoreGitTools.Contains(f.Name)).ToList(); - - var skillsResult = ReplSkillsLoader.BuildSkillsDetailed(ReplSkillsLoader.GetDefaultSearchDirs()); - skillsPlugin = skillsResult.Plugin; - skillsCatalog = skillsResult.CatalogBlock; - if (skillsPlugin is not null) - toolsByCategory["Skills"] = PluginRegistry.GetFunctionsFromObject(skillsPlugin).ToList(); - foreach (var warning in skillsResult.Warnings) - AnsiConsole.MarkupLine($"[yellow]⚠[/] {Markup.Escape(warning)}"); } var initialTools = toolsByCategory.Values.SelectMany(v => v).ToList(); @@ -242,6 +235,19 @@ protected override async Task ExecuteAsync( return 1; } + if (!settings.NoTools) + { + // Skill discovery/parsing/validation and the load_skill/read_skill_resource/ + // run_skill_script tools all come from Microsoft.Agents.AI's AgentFileSkillsSource/ + // AgentSkillsProvider — the same classes orchestration uses — via a throwaway + // ChatClientAgent wrapping the client just built above. + var skillsResult = await ReplSkillsLoader.BuildAsync(client, loggerFactory, cancellationToken); + discoveredSkills = skillsResult.Skills; + skillsCatalog = skillsResult.CatalogInstructions; + if (skillsResult.Tools.Count > 0) + toolsByCategory["Skills"] = skillsResult.Tools.ToList(); + } + var cwd = Directory.GetCurrentDirectory(); var eventsPath = FuseraftPaths.ExpandProjectPaths(FuseraftPaths.LocalReplEventsLog, FuseraftPaths.ProjectSlug(cwd)); @@ -377,7 +383,7 @@ protected override async Task ExecuteAsync( MessageRenderer.RenderReplHeader( modelId, cwd, pluginNames, sessionId, memoryCount: memoryEntries.Count, - skillCount: skillsPlugin?.Count ?? 0, + skillCount: discoveredSkills.Count, branch: TryGetGitBranch(cwd), eventsPath: settings.Verbose ? eventsPath : null); } @@ -388,10 +394,10 @@ protected override async Task ExecuteAsync( memoryStore, toolsByCategory, systemPrompt, pendingSave, verbose: settings.Verbose, subAgent: subAgent) { - JsonMode = jsonMode, - SkillsPlugin = skillsPlugin, - Todo = todoPlugin, - KeyStored = keyStored, + JsonMode = jsonMode, + Skills = discoveredSkills, + Todo = todoPlugin, + KeyStored = keyStored, }; if (!settings.NoTools) @@ -410,8 +416,8 @@ protected override async Task ExecuteAsync( .FirstOrDefault(); } - if (skillsPlugin is not null) - ctx.LineReader.SetSkillSlugs([.. skillsPlugin.Slugs]); + if (discoveredSkills.Count > 0) + ctx.LineReader.SetSkillSlugs([.. discoveredSkills.Select(s => s.Frontmatter.Name)]); // Wire the compact_context and get_context_status tools now that ctx is available. replSessionPlugin?.SetCompactDelegate(async (focus, ct) => diff --git a/src/Cli/Commands/Repl/ReplSessionContext.cs b/src/Cli/Commands/Repl/ReplSessionContext.cs index 2aee84bc..52c6a53d 100644 --- a/src/Cli/Commands/Repl/ReplSessionContext.cs +++ b/src/Cli/Commands/Repl/ReplSessionContext.cs @@ -1,3 +1,4 @@ +using Microsoft.Agents.AI; using Microsoft.Extensions.AI; using fuseraft.Core.Models; using fuseraft.Infrastructure; @@ -38,7 +39,7 @@ internal sealed class ReplSessionContext public readonly Dictionary> ToolsByCategory; public readonly SubAgentPlugin? SubAgent; public readonly bool Verbose; - public SkillsPlugin? SkillsPlugin { get; set; } + public IReadOnlyList Skills { get; set; } = []; public TodoPlugin? Todo { get; set; } // Mutable provider state (may be replaced by /provider setup) diff --git a/src/Cli/Commands/Repl/ReplSkillsLoader.cs b/src/Cli/Commands/Repl/ReplSkillsLoader.cs index 5907f742..70220dfd 100644 --- a/src/Cli/Commands/Repl/ReplSkillsLoader.cs +++ b/src/Cli/Commands/Repl/ReplSkillsLoader.cs @@ -1,255 +1,76 @@ -using System.Text; -using fuseraft.Core; +using Microsoft.Agents.AI; +using Microsoft.Extensions.AI; +using Microsoft.Extensions.Logging; using fuseraft.Core.Skills; -using fuseraft.Infrastructure.Plugins; namespace fuseraft.Cli.Commands.Repl; -/// Full result of a skill-directory scan: the catalog plugin plus any diagnostics. -/// The assembled , or null when no skills were found. -/// Catalog text for the REPL system prompt, or null when no skills were found. -/// -/// Human-readable reasons a discovered SKILL.md was skipped — always because it declared -/// a name:, description:, or compatibility: field that violates the Agent -/// Skills specification (). A skill that omits -/// frontmatter entirely is never skipped — see the "leniency" note on . -/// -internal sealed record SkillsLoadResult( - SkillsPlugin? Plugin, - string? CatalogBlock, - IReadOnlyList Warnings); +/// Result of wiring up skills for a REPL session. +/// Every skill discovered, for the startup banner count and $slug direct invocation. +/// Catalog text to append to the system prompt, or null when no skills were found. +/// The load_skill/read_skill_resource/run_skill_script tools, or empty when no skills were found. +internal sealed record ReplSkillsResult( + IReadOnlyList Skills, + string? CatalogInstructions, + IReadOnlyList Tools); /// -/// Scans skill directories, parses SKILL.md frontmatter, and assembles the -/// instance and catalog block injected into the REPL -/// system prompt at startup. +/// Thin REPL-side wiring over Microsoft.Agents.AI's Agent Skills feature. Discovery, frontmatter +/// parsing/validation, and the skill tools themselves all come from +/// / — the same classes +/// orchestration () uses, so a skill is treated +/// identically by both surfaces. This file does not parse or validate anything itself. /// internal static class ReplSkillsLoader { - // Matches orchestration's AgentFileSkillsSource search depth: root (0), skill dir (1), - // an optional one level of vendor namespacing (2). Bounded so a search dir pointed at a - // large or cyclic tree can't cause a runaway scan. - private const int MaxSkillSearchDepth = 2; + /// Convenience overload used by — searches the default dirs. + internal static Task BuildAsync( + IChatClient client, ILoggerFactory loggerFactory, CancellationToken cancellationToken) => + BuildAsync(client, loggerFactory, FuseraftSkillsSources.GetDefaultSearchDirs(), cancellationToken); /// - /// Returns the priority-ordered list of directories to scan for skills in a - /// normal REPL session (project-local → user-global → install-bundled). + /// Discovers skills under using + /// (wrapped in a throwaway — the only role it plays is + /// satisfying the framework's generic "which agent is asking" context, since file-based + /// discovery never invokes it) and returns the discovered skills plus the catalog + /// instructions and tools an would attach to that agent. /// - internal static string[] GetDefaultSearchDirs() + internal static async Task BuildAsync( + IChatClient client, ILoggerFactory loggerFactory, IEnumerable searchDirs, CancellationToken cancellationToken) { - var cwd = Directory.GetCurrentDirectory(); - var home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - return - [ - Path.Combine(cwd, ".fuseraft", "skills"), - Path.Combine(cwd, ".agents", "skills"), - FuseraftPaths.GlobalSkills, - Path.Combine(home, ".agents", "skills"), - Path.Combine(AppContext.BaseDirectory, "skills"), - ]; - } - - /// - /// Convenience overload used by — searches the default dirs. - /// - internal static (SkillsPlugin? Plugin, string? CatalogBlock) BuildSkills() => - BuildSkills(GetDefaultSearchDirs()); - - /// - /// Scans for SKILL.md files and returns a - /// together with a catalog string suitable for appending to the - /// REPL system prompt. Discards any skip warnings — see - /// for a caller that wants them. - /// - /// - /// Leniency: a SKILL.md with no frontmatter at all (or frontmatter with none - /// of the recognized fields) is still loaded, using its directory name as the slug — this - /// REPL surface does not require a name:/description: field the way fuseraft's - /// orchestration skills provider does. But when a name:, description:, or - /// compatibility: field is declared, it is validated against the same rules - /// orchestration enforces, and a violation (most commonly name: not matching the - /// directory name) skips the skill — silently accepting it here would let a skill work in - /// the REPL while remaining invisible to fuseraft run orchestration sessions. - /// - /// - internal static (SkillsPlugin? Plugin, string? CatalogBlock) BuildSkills(IEnumerable searchDirs) - { - var result = BuildSkillsDetailed(searchDirs); - return (result.Plugin, result.CatalogBlock); - } - - /// Same scan as , but also returns skip warnings. - internal static SkillsLoadResult BuildSkillsDetailed(IEnumerable searchDirs) - { - // slug → directory containing SKILL.md; first occurrence across searchDirs wins. - var skillDirs = new Dictionary(StringComparer.OrdinalIgnoreCase); - var descriptions = new Dictionary(StringComparer.OrdinalIgnoreCase); - var compatibility = new Dictionary(StringComparer.OrdinalIgnoreCase); - var warnings = new List(); - - foreach (var searchDir in searchDirs.Where(Directory.Exists)) - { - List skillMds; - try - { - skillMds = FindSkillMdFiles(searchDir); - } - catch (UnauthorizedAccessException) { continue; } - catch (IOException) { continue; } - - foreach (var skillMd in skillMds) - { - var skillDir = Path.GetDirectoryName(skillMd); - if (skillDir is null) continue; - var dirName = Path.GetFileName(skillDir); - if (string.IsNullOrEmpty(dirName) || skillDirs.ContainsKey(dirName)) continue; - - string content; - try - { - content = File.ReadAllText(skillMd); - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { continue; } - - if (!TryValidateFrontmatter(content, skillDir, dirName, warnings, - out var description, out var compat)) - continue; - - skillDirs[dirName] = skillDir; - descriptions[dirName] = description; - compatibility[dirName] = compat; - } - } - - if (skillDirs.Count == 0) return new SkillsLoadResult(null, null, warnings); - - var sb = new StringBuilder(); - sb.AppendLine("## SKILLS available"); - foreach (var slug in skillDirs.Keys.OrderBy(s => s, StringComparer.OrdinalIgnoreCase)) - { - var desc = descriptions.GetValueOrDefault(slug); - var compat = compatibility.GetValueOrDefault(slug); - var line = !string.IsNullOrWhiteSpace(desc) ? $"- {slug}: {desc}" : $"- {slug}"; - if (!string.IsNullOrWhiteSpace(compat)) - line += $" [requires: {compat}]"; - sb.AppendLine(line); - } - sb.AppendLine(); - sb.Append("Call load_skill(\"\") to get full step-by-step instructions before applying a skill."); - - return new SkillsLoadResult(new SkillsPlugin(skillDirs), sb.ToString(), warnings); - } - - /// - /// Parses 's frontmatter and validates any spec-covered field that - /// is actually present. Returns false (and appends a warning) only when a declared - /// field violates the spec — a skill with no frontmatter, or frontmatter missing these - /// fields entirely, always passes (see the leniency note on ). - /// - private static bool TryValidateFrontmatter( - string content, string skillDir, string dirName, List warnings, - out string? description, out string? compatibility) - { - description = null; - compatibility = null; - - var fm = SkillFrontmatterSpec.TryParse(content); - if (fm is null) return true; - - if (!string.IsNullOrEmpty(fm.Name)) - { - if (!SkillFrontmatterSpec.ValidateName(fm.Name, out var nameReason)) - { - warnings.Add($"Skipped skill at '{skillDir}': {nameReason}"); - return false; - } - if (!string.Equals(fm.Name, dirName, StringComparison.Ordinal)) - { - warnings.Add( - $"Skipped skill at '{skillDir}': name '{fm.Name}' does not match its directory " + - $"name '{dirName}' (this skill would also be invisible to 'fuseraft run' orchestration sessions)."); - return false; - } - } - - if (!string.IsNullOrEmpty(fm.Description)) - { - if (!SkillFrontmatterSpec.ValidateDescription(fm.Description, out var descReason)) - { - warnings.Add($"Skipped skill at '{skillDir}': {descReason}"); - return false; - } - description = fm.Description; - } - - if (!SkillFrontmatterSpec.ValidateCompatibility(fm.Compatibility, out var compatReason)) - { - warnings.Add($"Skipped skill at '{skillDir}': {compatReason}"); - return false; - } - compatibility = fm.Compatibility; - - return true; - } - - /// - /// Finds every SKILL.md under , recursing at most - /// levels and refusing to follow symlinked directories — - /// unbounded, symlink-following recursion could otherwise be tricked (via a symlink planted - /// in a project-controlled search dir) into scanning arbitrary parts of the filesystem, or - /// hang on a symlink cycle. Once a directory yields a SKILL.md, its subdirectories are - /// treated as part of that skill (references/scripts/assets), not as independent skill roots. - /// - private static List FindSkillMdFiles(string root) - { - var results = new List(); - FindSkillMdFiles(root, results, depth: 0); - return results; - } - - private static void FindSkillMdFiles(string directory, List results, int depth) - { - var candidate = Path.Combine(directory, "SKILL.md"); - if (File.Exists(candidate)) - { - if (!SkillPathGuard.IsReparsePoint(candidate)) - results.Add(candidate); - return; - } - - if (depth >= MaxSkillSearchDepth) return; - - IEnumerable subdirs; - try - { - subdirs = Directory.EnumerateDirectories(directory); - } - catch (Exception ex) when (ex is UnauthorizedAccessException or IOException) { return; } - - foreach (var sub in subdirs) - { - if (SkillPathGuard.IsReparsePoint(sub)) continue; - FindSkillMdFiles(sub, results, depth + 1); - } - } - - /// - /// Reads only the description: field from a SKILL.md YAML frontmatter block. - /// Returns null when the field is absent, empty, or the file is unreadable. - /// Kept as a thin wrapper over for callers that only need - /// the description of a single known file. - /// - internal static string? ParseSkillDescription(string skillMdPath) - { - try - { - var content = File.ReadAllText(skillMdPath); - var fm = SkillFrontmatterSpec.TryParse(content); - return string.IsNullOrWhiteSpace(fm?.Description) ? null : fm.Description; - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - return null; - } + var fileSource = new AgentFileSkillsSource( + searchDirs, + FuseraftSkillsSources.RunScriptAsync, + loggerFactory: loggerFactory); + + // Same caching+dedup pipeline AgentSkillsProvider's own convenience constructor builds + // internally — applied explicitly here so the skill list used for the startup banner + // count and $slug direct invocation agrees with what the catalog/tools below show, + // rather than the raw file source's un-deduplicated, per-search-dir concatenation. + var source = new DeduplicatingAgentSkillsSource(new CachingAgentSkillsSource(fileSource), loggerFactory); + + var agent = new ChatClientAgent(client); + IReadOnlyList skills = [.. await source.GetSkillsAsync(new AgentSkillsSourceContext(agent, session: null), cancellationToken)]; + + if (skills.Count == 0) + return new ReplSkillsResult(skills, null, []); + + var provider = new AgentSkillsProviderBuilder() + .UseSource(source) + .UseOptions(FuseraftSkillsSources.DisableApproval) + .UseLoggerFactory(loggerFactory) + .Build(); + + // AIContextProvider.InvokingContext is [Experimental] (MAAI001) as of the + // Microsoft.Agents.AI version fuseraft depends on — see the same suppression pattern + // in AgentContextCompactionFilters.cs. This is the only place that touches it. +#pragma warning disable MAAI001 + var aiContext = await provider.InvokingAsync( + new AIContextProvider.InvokingContext(agent, session: null, aiContext: new AIContext()), + cancellationToken); +#pragma warning restore MAAI001 + + var tools = aiContext.Tools?.OfType().ToList() ?? []; + return new ReplSkillsResult(skills, aiContext.Instructions, tools); } } diff --git a/src/Cli/Commands/Repl/ReplTurn.cs b/src/Cli/Commands/Repl/ReplTurn.cs index b5fdcaa6..51049852 100644 --- a/src/Cli/Commands/Repl/ReplTurn.cs +++ b/src/Cli/Commands/Repl/ReplTurn.cs @@ -322,10 +322,11 @@ await ExecuteAsync( var slug = parts[0][1..]; // strip '$' var args = parts.Length > 1 ? parts[1] : string.Empty; - if (ctx.SkillsPlugin is null || !ctx.SkillsPlugin.HasSkill(slug)) + var skill = ctx.Skills.FirstOrDefault(s => string.Equals(s.Frontmatter.Name, slug, StringComparison.OrdinalIgnoreCase)); + if (skill is null) { - var available = ctx.SkillsPlugin is not null - ? $"Available: {string.Join(", ", ctx.SkillsPlugin.Slugs.Take(10))}" + var available = ctx.Skills.Count > 0 + ? $"Available: {string.Join(", ", ctx.Skills.Select(s => s.Frontmatter.Name).Take(10))}" : "No skills are loaded in this session."; var errMsg = string.IsNullOrEmpty(slug) ? $"Usage: $ [args]. {available}" @@ -337,7 +338,7 @@ await ExecuteAsync( continue; } - var skillContent = await ctx.SkillsPlugin.LoadSkillAsync(slug, cancellationToken); + var skillContent = await skill.GetContentAsync(cancellationToken); var input = string.IsNullOrEmpty(args) ? skillContent : $"{skillContent}\n\n{args}"; await ExecuteAsync(ctx, input, isStepRequest: false, capturePlan: false, activeStep: null, cancellationToken); diff --git a/src/Cli/Commands/Skills/SkillsAddCommand.cs b/src/Cli/Commands/Skills/SkillsAddCommand.cs index 83248868..dc3d04f9 100644 --- a/src/Cli/Commands/Skills/SkillsAddCommand.cs +++ b/src/Cli/Commands/Skills/SkillsAddCommand.cs @@ -1,4 +1,5 @@ using System.ComponentModel; +using Microsoft.Agents.AI; using Spectre.Console; using Spectre.Console.Cli; using fuseraft.Core; @@ -55,26 +56,12 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsAd return 1; } - if (slug.Length > SkillFrontmatterSpec.MaxNameLength) - { - slug = slug[..SkillFrontmatterSpec.MaxNameLength].TrimEnd('-'); - AnsiConsole.MarkupLine( - $"[yellow]⚠[/] Derived name exceeds {SkillFrontmatterSpec.MaxNameLength} characters; truncated to [bold]{Markup.Escape(slug)}[/]."); - } - // Guarantee the installed file's 'name:' field matches the directory it's installed // under — a raw name that needed slugifying (spaces, uppercase, ...) would otherwise // leave the two disagreeing, which works fine in the REPL's lenient loader but is // silently dropped by fuseraft's orchestration skills provider. content = SkillsHelpers.CanonicalizeName(content, slug); - if (!SkillFrontmatterSpec.ValidateDescription(SkillsHelpers.ExtractDescription(content), out var descReason)) - { - AnsiConsole.MarkupLine( - $"[yellow]⚠[/] {Markup.Escape(descReason!)} " + - "This skill will work in the REPL but 'fuseraft run' orchestration sessions will silently drop it."); - } - var destDir = Path.Combine(FuseraftPaths.GlobalSkills, slug); var destPath = Path.Combine(destDir, "SKILL.md"); var isUpdate = File.Exists(destPath); @@ -106,6 +93,20 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsAd var verb = isUpdate ? "Updated" : "Added"; AnsiConsole.MarkupLine($"[green]✓[/] {verb} [bold]{Markup.Escape(slug)}[/] → {Markup.Escape(destPath)}"); + + // Canonicalizing the name only guarantees name-matches-directory; description/ + // compatibility could still be missing or too long. Confirm with the same + // AgentFileSkillsSource pipeline orchestration and the REPL actually use, rather than + // re-deriving the answer here. + var checkSource = new AgentFileSkillsSource(destDir, FuseraftSkillsSources.RunScriptAsync); + var checkResult = await checkSource.GetSkillsAsync( + new AgentSkillsSourceContext(SkillDiscoveryAgent.Create(), session: null), cancellationToken); + if (checkResult.Count == 0) + AnsiConsole.MarkupLine( + $"[yellow]⚠[/] '{Markup.Escape(slug)}' does not fully conform to the Agent Skills specification " + + $"(name matches its directory, but check description/compatibility). It will work in the REPL but " + + $"'fuseraft run' orchestration sessions will silently drop it — run [bold]fuseraft skills validate {Markup.Escape(slug)}[/] for details."); + return 0; } } diff --git a/src/Cli/Commands/Skills/SkillsHelpers.cs b/src/Cli/Commands/Skills/SkillsHelpers.cs index fc15811e..3066a851 100644 --- a/src/Cli/Commands/Skills/SkillsHelpers.cs +++ b/src/Cli/Commands/Skills/SkillsHelpers.cs @@ -1,35 +1,68 @@ +using System.Text.RegularExpressions; using fuseraft.Core.Skills; namespace fuseraft.Cli.Commands.Skills; +/// +/// Bootstrapping helpers exclusive to fuseraft skills add, which — unlike every other +/// skill-related surface (REPL, orchestration, skills validate, skills list, +/// , all of which use +/// Microsoft.Agents.AI's AgentFileSkillsSource/AgentSkillFrontmatter directly) — +/// intentionally stays lenient: it derives an install slug from a raw title (spaces, uppercase, +/// ...) and rewrites the installed copy's name: field to match, rather than requiring the +/// source to already be spec-compliant. See docs/skills.md. +/// internal static class SkillsHelpers { + private static readonly Regex SlugSanitizer = new(@"[^a-z0-9]+", RegexOptions.Compiled); + /// Extracts the slugified name: field, or null when absent/empty. internal static string? ExtractSlug(string content) { - var name = SkillFrontmatterSpec.TryParse(content)?.Name; + var name = FrontmatterFieldReader.ExtractField(content, "name"); return string.IsNullOrWhiteSpace(name) ? null : ToSlug(name); } - internal static string ExtractDescription(string content) => - SkillFrontmatterSpec.TryParse(content)?.Description ?? string.Empty; - - internal static string ToSlug(string name) => SkillFrontmatterSpec.ToSlug(name); + /// Converts an arbitrary title into a spec-valid slug candidate: lowercase, non-alphanumeric runs collapsed to single hyphens, no leading/trailing hyphens. + internal static string ToSlug(string name) => + SlugSanitizer.Replace(name.ToLowerInvariant().Trim(), "-").Trim('-'); /// /// Rewrites 's name: frontmatter field to - /// when it isn't already exactly that value. Ensures a skill - /// installed under <slug>/SKILL.md always has a matching name: field — - /// without this, a raw name that needed slugifying (spaces, uppercase, etc.) would leave the - /// installed file internally inconsistent: fine in the REPL's lenient loader, but silently - /// dropped by fuseraft's orchestration skills provider, which requires an exact match. + /// when it isn't already exactly that value (inserting one if the + /// field was missing entirely). Ensures a skill installed under <slug>/SKILL.md + /// always has a matching name: field — without this, a raw title that needed + /// slugifying would leave the installed file internally inconsistent: fine in the REPL's + /// lenient loader, but rejected by AgentFileSkillsSource's name-matches-directory + /// check, which orchestration and skills validate both enforce. /// internal static string CanonicalizeName(string content, string slug) { - var currentName = SkillFrontmatterSpec.TryParse(content)?.Name; - return string.Equals(currentName, slug, StringComparison.Ordinal) - ? content - : SkillFrontmatterSpec.WithCanonicalName(content, slug); + var currentName = FrontmatterFieldReader.ExtractField(content, "name"); + if (string.Equals(currentName, slug, StringComparison.Ordinal)) + return content; + + var frontmatterMatch = Regex.Match(content, @"\A^---\s*$(.*?)^---\s*$", RegexOptions.Multiline | RegexOptions.Singleline); + if (!frontmatterMatch.Success) + return content; + + var yaml = frontmatterMatch.Groups[1].Value; + var nameLine = $"name: {slug}"; + + string newYaml; + var existingNameLine = Regex.Match(yaml, @"^name\s*:[ \t]*.*$", RegexOptions.Multiline | RegexOptions.IgnoreCase); + if (existingNameLine.Success) + { + newYaml = yaml[..existingNameLine.Index] + nameLine + yaml[(existingNameLine.Index + existingNameLine.Length)..]; + } + else + { + // The captured yaml group starts right after "---" and before its own trailing + // newline (the regex's '$' anchor is zero-width), so it always begins with '\n'. + newYaml = "\n" + nameLine + "\n" + yaml.TrimStart('\n'); + } + + return content[..frontmatterMatch.Groups[1].Index] + newYaml + content[(frontmatterMatch.Groups[1].Index + frontmatterMatch.Groups[1].Length)..]; } /// diff --git a/src/Cli/Commands/Skills/SkillsListCommand.cs b/src/Cli/Commands/Skills/SkillsListCommand.cs index f818f502..7c0972e0 100644 --- a/src/Cli/Commands/Skills/SkillsListCommand.cs +++ b/src/Cli/Commands/Skills/SkillsListCommand.cs @@ -1,4 +1,5 @@ using System.ComponentModel; +using Microsoft.Agents.AI; using Spectre.Console; using Spectre.Console.Cli; using fuseraft.Core; @@ -22,25 +23,23 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsLi return 0; } - var entries = new List<(string Slug, string Description, string? Compatibility, bool Valid)>(); - foreach (var dir in Directory.EnumerateDirectories(root).OrderBy(d => d)) - { - var mdPath = Path.Combine(dir, "SKILL.md"); - if (!File.Exists(mdPath)) continue; - var content = await File.ReadAllTextAsync(mdPath, cancellationToken); - var slug = Path.GetFileName(dir); - var frontmatter = SkillFrontmatterSpec.TryParse(content); - var desc = frontmatter?.Description ?? string.Empty; - var valid = SkillFrontmatterSpec.Validate(frontmatter, slug).Count == 0; - entries.Add((slug, desc, frontmatter?.Compatibility, valid)); - } + var dirs = Directory.EnumerateDirectories(root) + .Where(d => File.Exists(Path.Combine(d, "SKILL.md"))) + .OrderBy(d => d, StringComparer.OrdinalIgnoreCase) + .ToList(); - if (entries.Count == 0) + if (dirs.Count == 0) { AnsiConsole.MarkupLine("[dim]No skills installed. Use [bold]fuseraft skills add [/] to add one.[/]"); return 0; } + // The same discovery pipeline the REPL and orchestration use at runtime — a skill shown + // here with a real description/compatibility is guaranteed to load identically in both. + var source = new AgentFileSkillsSource(root, FuseraftSkillsSources.RunScriptAsync); + var skills = await source.GetSkillsAsync(new AgentSkillsSourceContext(SkillDiscoveryAgent.Create(), session: null), cancellationToken); + var bySlug = skills.ToDictionary(s => s.Frontmatter.Name, StringComparer.Ordinal); + var table = new Table() .Border(TableBorder.Simple) .AddColumn(new TableColumn("[bold]Slug[/]")) @@ -48,19 +47,20 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsLi .AddColumn(new TableColumn("[bold]Requires[/]")) .AddColumn(new TableColumn("[bold]Spec[/]")); - foreach (var (slug, desc, compatibility, valid) in entries) + foreach (var dir in dirs) { - var specMark = valid ? "[green]✓[/]" : "[red]✗[/]"; + var slug = Path.GetFileName(dir); + var valid = bySlug.TryGetValue(slug, out var skill); table.AddRow( Markup.Escape(slug), - Markup.Escape(desc), - Markup.Escape(compatibility ?? ""), - specMark); + Markup.Escape(valid ? skill!.Frontmatter.Description : ""), + Markup.Escape(valid ? skill!.Frontmatter.Compatibility ?? "" : ""), + valid ? "[green]✓[/]" : "[red]✗[/]"); } AnsiConsole.Write(table); - AnsiConsole.MarkupLine($"[dim]{entries.Count} skill(s) in {Markup.Escape(root)}[/]"); - if (entries.Any(e => !e.Valid)) + AnsiConsole.MarkupLine($"[dim]{dirs.Count} skill(s) in {Markup.Escape(root)}[/]"); + if (bySlug.Count < dirs.Count) AnsiConsole.MarkupLine("[dim]Run [bold]fuseraft skills validate[/] for details on the ✗ entries.[/]"); return 0; } diff --git a/src/Cli/Commands/Skills/SkillsValidateCommand.cs b/src/Cli/Commands/Skills/SkillsValidateCommand.cs index bde68c01..f66f9d0b 100644 --- a/src/Cli/Commands/Skills/SkillsValidateCommand.cs +++ b/src/Cli/Commands/Skills/SkillsValidateCommand.cs @@ -1,4 +1,5 @@ using System.ComponentModel; +using Microsoft.Agents.AI; using Spectre.Console; using Spectre.Console.Cli; using fuseraft.Core; @@ -18,15 +19,19 @@ public sealed class SkillsValidateSettings : CommandSettings /// /// fuseraft's equivalent of the skills-ref validate tool the Agent Skills specification /// () recommends authors run before -/// shipping a skill — checks a SKILL.md's frontmatter against every naming and field-length rule -/// the spec defines, using the exact same validator fuseraft's orchestration skills provider -/// applies at load time. +/// shipping a skill. The pass/fail verdict comes from — the +/// same discovery pipeline the REPL and orchestration both use at runtime, so a skill that +/// passes here is guaranteed to load identically in both. Per-failure reasons come from +/// 's own validating constructor, fed by the minimal raw +/// name:/description:/compatibility: extraction in +/// — nothing here re-implements the specification's rules. /// public sealed class SkillsValidateCommand : AsyncCommand { protected override async Task ExecuteAsync(CommandContext context, SkillsValidateSettings settings, CancellationToken cancellationToken) { - List skillDirs; + string searchRoot; + List candidateDirs; if (!string.IsNullOrWhiteSpace(settings.Path)) { @@ -36,21 +41,28 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsVa AnsiConsole.MarkupLine($"[red]✗ Not a directory: {Markup.Escape(settings.Path)}[/]"); return 1; } - skillDirs = [dir.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)]; + searchRoot = dir; + candidateDirs = [Normalize(dir)]; } else { - var root = FuseraftPaths.GlobalSkills; - if (!Directory.Exists(root)) + searchRoot = FuseraftPaths.GlobalSkills; + if (!Directory.Exists(searchRoot)) { AnsiConsole.MarkupLine("[dim]No skills installed. Use [bold]fuseraft skills add [/] to add one.[/]"); return 0; } - skillDirs = Directory.EnumerateDirectories(root).OrderBy(d => d, StringComparer.OrdinalIgnoreCase).ToList(); + candidateDirs = [.. Directory.EnumerateDirectories(searchRoot).Select(Normalize).OrderBy(d => d, StringComparer.OrdinalIgnoreCase)]; } + var source = new AgentFileSkillsSource(searchRoot, FuseraftSkillsSources.RunScriptAsync); + var passed = await source.GetSkillsAsync(new AgentSkillsSourceContext(SkillDiscoveryAgent.Create(), session: null), cancellationToken); + var passedByDir = passed + .OfType() + .ToDictionary(s => Normalize(s.Path), s => s, StringComparer.OrdinalIgnoreCase); + var allValid = true; - foreach (var dir in skillDirs) + foreach (var dir in candidateDirs) { var name = Path.GetFileName(dir); var skillMd = Path.Combine(dir, "SKILL.md"); @@ -62,11 +74,7 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsVa continue; } - var content = await File.ReadAllTextAsync(skillMd, cancellationToken); - var frontmatter = SkillFrontmatterSpec.TryParse(content); - var violations = SkillFrontmatterSpec.Validate(frontmatter, name); - - if (violations.Count == 0) + if (passedByDir.ContainsKey(dir)) { AnsiConsole.MarkupLine($"[green]✓[/] [bold]{Markup.Escape(name)}[/]"); continue; @@ -74,7 +82,7 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsVa allValid = false; AnsiConsole.MarkupLine($"[red]✗[/] [bold]{Markup.Escape(name)}[/]"); - foreach (var violation in violations) + foreach (var violation in await DescribeViolationsAsync(skillMd, name, cancellationToken)) AnsiConsole.MarkupLine($" [red]•[/] {Markup.Escape(violation)}"); } @@ -85,4 +93,41 @@ protected override async Task ExecuteAsync(CommandContext context, SkillsVa return allValid ? 0 : 1; } + + /// + /// Explains why a skill directory that silently dropped + /// failed, by handing the same raw name:/description:/compatibility: + /// values to 's own validating constructor and reporting + /// its exception message (or a name/directory mismatch, the one thing that constructor + /// doesn't check since it has no notion of a directory). + /// + private static async Task> DescribeViolationsAsync(string skillMdPath, string directoryName, CancellationToken cancellationToken) + { + var content = await File.ReadAllTextAsync(skillMdPath, cancellationToken); + var rawName = FrontmatterFieldReader.ExtractField(content, "name"); + var rawDescription = FrontmatterFieldReader.ExtractField(content, "description"); + var rawCompatibility = FrontmatterFieldReader.ExtractField(content, "compatibility"); + + if (rawName is null && rawDescription is null) + return ["No YAML frontmatter block found (SKILL.md must start with a '---' delimited block with 'name:' and 'description:' fields)."]; + + var violations = new List(); + AgentSkillFrontmatter? frontmatter = null; + try + { + frontmatter = new AgentSkillFrontmatter(rawName ?? string.Empty, rawDescription ?? string.Empty, rawCompatibility); + } + catch (ArgumentException ex) + { + violations.Add(ex.Message); + } + + if (frontmatter is not null && !string.Equals(frontmatter.Name, directoryName, StringComparison.Ordinal)) + violations.Add($"'name: {frontmatter.Name}' does not match its directory name '{directoryName}'."); + + return violations.Count > 0 ? violations : ["Does not conform to the Agent Skills specification (reason unknown — check for stray YAML syntax)."]; + } + + private static string Normalize(string path) => + Path.GetFullPath(path).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); } diff --git a/src/Cli/OrchestratorBuilder.cs b/src/Cli/OrchestratorBuilder.cs index bc5b83ee..39f02049 100644 --- a/src/Cli/OrchestratorBuilder.cs +++ b/src/Cli/OrchestratorBuilder.cs @@ -16,6 +16,7 @@ using fuseraft.Core; using fuseraft.Core.Interfaces; using fuseraft.Core.Models; +using fuseraft.Core.Skills; using fuseraft.Infrastructure; using fuseraft.Infrastructure.KeyStore; using fuseraft.Infrastructure.Plugins; @@ -1466,18 +1467,8 @@ private static (IOrchestrator Orchestrator, fuseraft.Infrastructure.Repository.R private static AgentSkillsProvider? BuildSkillsProvider(ILoggerFactory loggerFactory) { - // Project-native → project cross-client → user-native → user cross-client → built-in. - var home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); - var dirs = new[] - { - Path.Combine(Directory.GetCurrentDirectory(), ".fuseraft", "skills"), - Path.Combine(Directory.GetCurrentDirectory(), ".agents", "skills"), - FuseraftPaths.GlobalSkills, - Path.Combine(home, ".agents", "skills"), - Path.Combine(AppContext.BaseDirectory, "skills"), - }.Where(Directory.Exists).ToArray(); - - if (dirs.Length == 0) return null; + var dirs = FuseraftSkillsSources.GetDefaultSearchDirs(); + if (!dirs.Any(Directory.Exists)) return null; // Without a logger factory, AgentFileSkillsSource discards its diagnostics (invalid // frontmatter, a skill 'name:' that doesn't match its directory name, symlink/path- @@ -1486,60 +1477,9 @@ private static (IOrchestrator Orchestrator, fuseraft.Infrastructure.Repository.R // pipeline as the rest of the orchestrator. return new AgentSkillsProviderBuilder() .UseFileSkills(dirs) - .UseFileScriptRunner(RunSkillScriptAsync) + .UseFileScriptRunner(FuseraftSkillsSources.RunScriptAsync) + .UseOptions(FuseraftSkillsSources.DisableApproval) .UseLoggerFactory(loggerFactory) .Build(); } - - private static async Task RunSkillScriptAsync( - AgentFileSkill skill, - AgentFileSkillScript script, - JsonElement? arguments, - IServiceProvider? serviceProvider, - CancellationToken cancellationToken) - { - var ext = Path.GetExtension(script.FullPath).ToLowerInvariant(); - var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); - var program = ext switch - { - ".py" => isWindows ? "python" : "python3", - ".sh" => "bash", - ".js" => "node", - _ => null - }; - if (program is null) - return $"No runner registered for '{ext}' scripts."; - - var psi = new ProcessStartInfo - { - FileName = program, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - }; - psi.ArgumentList.Add(script.FullPath); - if (arguments.HasValue && arguments.Value.ValueKind == JsonValueKind.Object) - { - foreach (var prop in arguments.Value.EnumerateObject()) - { - var val = prop.Value.ToString(); - if (!string.IsNullOrEmpty(val)) - psi.ArgumentList.Add(val); - } - } - - using var proc = Process.Start(psi) - ?? throw new InvalidOperationException($"Failed to start {program}"); - - // Read stdout and stderr concurrently — sequential reads deadlock if either pipe fills. - var stdoutTask = proc.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = proc.StandardError.ReadToEndAsync(cancellationToken); - await Task.WhenAll(stdoutTask, stderrTask); - await proc.WaitForExitAsync(cancellationToken); - - var stdout = await stdoutTask; - var stderr = await stderrTask; - return string.IsNullOrWhiteSpace(stderr) ? stdout : $"{stdout}\nstderr: {stderr}"; - } - } diff --git a/src/Core/Skills/FrontmatterFieldReader.cs b/src/Core/Skills/FrontmatterFieldReader.cs new file mode 100644 index 00000000..3c76c861 --- /dev/null +++ b/src/Core/Skills/FrontmatterFieldReader.cs @@ -0,0 +1,61 @@ +using System.Text.RegularExpressions; + +namespace fuseraft.Core.Skills; + +/// +/// Reads a single top-level frontmatter field's raw value from a SKILL.md file's content — +/// nothing more. +/// +/// +/// This is the one remaining piece of hand-written skill-related code in fuseraft, and it +/// deliberately does no validation of its own. Every real spec question (is this name valid +/// kebab-case, does it match its directory, is the description within the length limit, ...) is +/// answered exclusively by Microsoft.Agents.AI's AgentSkillFrontmatter/ +/// AgentFileSkillsSource. Those classes have no public entry point that parses a raw +/// string outside of the full file-discovery pipeline, which itself requires the file to already +/// live at a directory whose name matches its own name: field — a chicken-and-egg problem +/// for the two places that need to know a candidate's intended name before it's placed +/// anywhere: fuseraft skills add (installing a skill whose source directory doesn't yet +/// match) and SkillCurator (writing a freshly-generated skill to disk for the first time). +/// This method exists solely to answer "what does this file currently call itself" for that +/// narrow bootstrapping purpose. +/// +/// +public static class FrontmatterFieldReader +{ + private static readonly TimeSpan RegexTimeout = TimeSpan.FromSeconds(2); + + private static readonly Regex FrontmatterBlock = + new(@"\A^---\s*$(.*?)^---\s*$", RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled, RegexTimeout); + + private static readonly Regex TopLevelKeyValue = + new(@"^([A-Za-z][\w-]*)\s*:[ \t]*(?:""([^""]*)""|'([^']*)'|(\S.*?))?\s*$", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); + + /// + /// Returns the raw value of a top-level line inside + /// 's YAML frontmatter block, or null when the frontmatter + /// block, the key, or its value is absent. + /// + public static string? ExtractField(string? content, string key) + { + if (string.IsNullOrEmpty(content)) return null; + + Match block; + try { block = FrontmatterBlock.Match(content); } + catch (RegexMatchTimeoutException) { return null; } + if (!block.Success) return null; + + foreach (Match m in TopLevelKeyValue.Matches(block.Groups[1].Value)) + { + if (!string.Equals(m.Groups[1].Value, key, StringComparison.OrdinalIgnoreCase)) continue; + + var value = m.Groups[2].Success ? m.Groups[2].Value + : m.Groups[3].Success ? m.Groups[3].Value + : m.Groups[4].Success ? m.Groups[4].Value + : null; + return string.IsNullOrWhiteSpace(value) ? null : value.Trim(); + } + + return null; + } +} diff --git a/src/Core/Skills/FuseraftSkillsSources.cs b/src/Core/Skills/FuseraftSkillsSources.cs new file mode 100644 index 00000000..1d1d37a8 --- /dev/null +++ b/src/Core/Skills/FuseraftSkillsSources.cs @@ -0,0 +1,158 @@ +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Text.Json; +using Microsoft.Agents.AI; +using Microsoft.Extensions.AI; + +namespace fuseraft.Core.Skills; + +/// +/// Shared plumbing for wiring up Microsoft.Agents.AI's Agent Skills feature +/// (/) — the single +/// implementation both the REPL (fuseraft repl) and orchestration (fuseraft run) +/// use for skill discovery, frontmatter parsing/validation, and progressive disclosure. Nothing +/// in this file parses or validates SKILL.md content; that is entirely Microsoft's +/// /. This file only +/// supplies the two things the library deliberately leaves to the host: where to search, and +/// how to execute a script file on this OS. +/// +public static class FuseraftSkillsSources +{ + /// + /// Priority-ordered directories both the REPL and orchestration scan for skills + /// (project-native → project cross-client → user-native → user cross-client → built-in). + /// Non-existent directories are skipped by itself. + /// + public static string[] GetDefaultSearchDirs() + { + var cwd = Directory.GetCurrentDirectory(); + var home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); + return + [ + Path.Combine(cwd, ".fuseraft", "skills"), + Path.Combine(cwd, ".agents", "skills"), + fuseraft.Core.FuseraftPaths.GlobalSkills, + Path.Combine(home, ".agents", "skills"), + Path.Combine(AppContext.BaseDirectory, "skills"), + ]; + } + + /// + /// Applies the provider options fuseraft needs regardless of caller: fuseraft has no + /// Microsoft.Agents.AI ToolApprovalAgentOptions pipeline wired up anywhere — + /// neither the REPL (which drives directly via + /// UseFunctionInvocation(), a plain Microsoft.Extensions.AI concept with no + /// approval semantics) nor orchestration (which has its own, unrelated + /// IHumanApprovalService for shell commands, never wired to skill tools). Leaving + /// 's default (approval required for all three tools) would + /// make load_skill/read_skill_resource/run_skill_script silently + /// non-functional rather than "safely gated" — a model's attempt to call them would come + /// back as an unresolved approval request that nothing in fuseraft ever grants. + /// + public static void DisableApproval(AgentSkillsProviderOptions options) + { + options.DisableLoadSkillApproval = true; + options.DisableReadSkillResourceApproval = true; + options.DisableRunSkillScriptApproval = true; + } + + /// + /// Runs a file-based skill script as a local subprocess. Ported from Microsoft's own + /// reference implementation (samples/02-agents/AgentSkills/SubprocessScriptRunner.cs + /// in the agent-framework repo, referenced directly from 's + /// own XML doc example) rather than reimplemented, since the framework does not ship a + /// default script runner — is an intentional + /// extension point the host must supply. + /// + public static async Task RunScriptAsync( + AgentFileSkill skill, + AgentFileSkillScript script, + JsonElement? arguments, + IServiceProvider? serviceProvider, + CancellationToken cancellationToken) + { + if (!File.Exists(script.FullPath)) + return $"Error: Script file not found: {script.FullPath}"; + + var extension = Path.GetExtension(script.FullPath); + var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); + string? interpreter = extension switch + { + ".py" => isWindows ? "python" : "python3", + ".js" => "node", + ".sh" => "bash", + ".ps1" => "pwsh", + _ => null, + }; + + var startInfo = new ProcessStartInfo + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + WorkingDirectory = Path.GetDirectoryName(script.FullPath) ?? ".", + }; + + if (interpreter is not null) + { + startInfo.FileName = interpreter; + startInfo.ArgumentList.Add(script.FullPath); + } + else + { + startInfo.FileName = script.FullPath; + } + + if (arguments is { ValueKind: JsonValueKind.Array } json) + { + foreach (var element in json.EnumerateArray()) + { + if (element.ValueKind != JsonValueKind.String) + throw new InvalidOperationException( + $"File-based skill scripts only accept string CLI arguments but received a JSON element of kind '{element.ValueKind}'."); + startInfo.ArgumentList.Add(element.GetString()!); + } + } + else if (arguments is not null && arguments.Value.ValueKind is not (JsonValueKind.Null or JsonValueKind.Undefined)) + { + throw new InvalidOperationException( + $"Expected a JSON array of CLI arguments but received {arguments.Value.ValueKind}."); + } + + Process? process = null; + try + { + process = Process.Start(startInfo); + if (process is null) + return $"Error: Failed to start process for script '{script.Name}'."; + + var outputTask = process.StandardOutput.ReadToEndAsync(cancellationToken); + var errorTask = process.StandardError.ReadToEndAsync(cancellationToken); + await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); + + var output = await outputTask.ConfigureAwait(false); + var error = await errorTask.ConfigureAwait(false); + + if (!string.IsNullOrEmpty(error)) + output += $"\nStderr:\n{error}"; + if (process.ExitCode != 0) + output += $"\nScript exited with code {process.ExitCode}"; + + return string.IsNullOrEmpty(output) ? "(no output)" : output.Trim(); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + process?.Kill(entireProcessTree: true); + throw; + } + catch (Exception ex) + { + return $"Error: Failed to execute script '{script.Name}': {ex.Message}"; + } + finally + { + process?.Dispose(); + } + } +} diff --git a/src/Core/Skills/SkillDiscoveryAgent.cs b/src/Core/Skills/SkillDiscoveryAgent.cs new file mode 100644 index 00000000..13970162 --- /dev/null +++ b/src/Core/Skills/SkillDiscoveryAgent.cs @@ -0,0 +1,38 @@ +using Microsoft.Agents.AI; +using Microsoft.Extensions.AI; + +namespace fuseraft.Core.Skills; + +/// +/// Provides a throwaway for CLI commands that need to call +/// (or ) +/// purely to validate or inspect skill files on disk, with no live model involved +/// (skills add, skills validate, skills list). +/// +/// +/// Both and +/// require a non-null handle, even though the file-based skills source never +/// reads anything from it — the parameter exists generically across all +/// implementations (an MCP-backed source, for instance, might scope skills per agent identity). +/// Where a real is already in hand (the REPL, SkillCurator), +/// wrap that instead of using this — this stub deliberately can never answer a real prompt. +/// +/// +public static class SkillDiscoveryAgent +{ + /// Creates a new throwaway agent backed by a chat client that is never actually invoked. + public static AIAgent Create() => new ChatClientAgent(new NonInvocableChatClient()); + + private sealed class NonInvocableChatClient : IChatClient + { + public Task GetResponseAsync(IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) => + throw new NotSupportedException($"{nameof(NonInvocableChatClient)} exists only to satisfy an API's AIAgent requirement for offline skill discovery and cannot answer prompts."); + + public IAsyncEnumerable GetStreamingResponseAsync(IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) => + throw new NotSupportedException($"{nameof(NonInvocableChatClient)} exists only to satisfy an API's AIAgent requirement for offline skill discovery and cannot answer prompts."); + + public object? GetService(Type serviceType, object? serviceKey = null) => null; + + public void Dispose() { } + } +} diff --git a/src/Core/Skills/SkillFrontmatter.cs b/src/Core/Skills/SkillFrontmatter.cs deleted file mode 100644 index 1d277dc5..00000000 --- a/src/Core/Skills/SkillFrontmatter.cs +++ /dev/null @@ -1,266 +0,0 @@ -using System.Text.RegularExpressions; - -namespace fuseraft.Core.Skills; - -/// -/// The YAML frontmatter fields of a SKILL.md file as defined by the -/// Agent Skills specification. -/// -/// Raw name: value, or empty string if absent. Not guaranteed valid — use . -/// Raw description: value, or empty string if absent. -/// Optional license: value. -/// Optional compatibility: value. -/// Optional allowed-tools: value (space-separated, experimental per spec). -/// Optional metadata: map of string keys to string values. -public sealed record SkillFrontmatter( - string Name, - string Description, - string? License, - string? Compatibility, - string? AllowedTools, - IReadOnlyDictionary? Metadata); - -/// -/// Single source of truth for parsing and validating SKILL.md frontmatter against the -/// Agent Skills specification (). -/// -/// -/// fuseraft has two separate skill-loading surfaces — the REPL's own hand-rolled loader -/// (ReplSkillsLoader/SkillsPlugin) and orchestration's Microsoft.Agents.AI -/// skills provider — that historically diverged in what they accepted. This type mirrors the -/// validation rules of Microsoft's AgentSkillFrontmatter exactly (same length limits, same -/// name regex) so both surfaces treat a given SKILL.md identically, and so the CLI-side commands -/// (skills add, skills validate, skill curation) can enforce the same rules before -/// ever writing a file to disk. -/// -/// -public static class SkillFrontmatterSpec -{ - public const int MaxNameLength = 64; - public const int MaxDescriptionLength = 1024; - public const int MaxCompatibilityLength = 500; - - private static readonly TimeSpan RegexTimeout = TimeSpan.FromSeconds(2); - - // Lowercase letters, numbers, and hyphens only; no leading/trailing/consecutive hyphens. - private static readonly Regex ValidNameRegex = - new(@"^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$", RegexOptions.Compiled, RegexTimeout); - - // Matches the YAML frontmatter block delimited by "---" lines. Callers strip a leading - // UTF-8 BOM (via TrimBom) before matching, since some editors prepend one. - private static readonly Regex FrontmatterBlock = - new(@"\A^---\s*$(.*?)^---\s*$", RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled, RegexTimeout); - - // Matches a top-level "key: value" line (no leading indentation) — double-quoted, - // single-quoted, or bare scalar values. - private static readonly Regex TopLevelKeyValue = - new(@"^([A-Za-z][\w-]*)\s*:[ \t]*(?:""([^""]*)""|'([^']*)'|(\S.*?))?\s*$", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); - - // Matches a "metadata:" line followed by one or more indented sub-lines. - private static readonly Regex MetadataBlock = - new(@"^metadata\s*:\s*$\r?\n((?:[ \t]+\S.*(?:\r?\n|\z))+)", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); - - // Matches an indented "key: value" line within a metadata block. - private static readonly Regex IndentedKeyValue = - new(@"^[ \t]+([A-Za-z][\w-]*)\s*:[ \t]*(?:""([^""]*)""|'([^']*)'|(\S.*?))?\s*$", RegexOptions.Multiline | RegexOptions.Compiled, RegexTimeout); - - private static readonly Regex SlugSanitizer = new(@"[^a-z0-9]+", RegexOptions.Compiled, RegexTimeout); - - /// - /// Parses the YAML frontmatter block from a SKILL.md file's content. Returns null - /// when there is no frontmatter block at all, or the block contains none of the recognized - /// fields. This is a raw parse — it does not validate the values; call - /// to check spec conformance. - /// - public static SkillFrontmatter? TryParse(string? content) - { - if (string.IsNullOrEmpty(content)) return null; - - Match block; - try { block = FrontmatterBlock.Match(TrimBom(content)); } - catch (RegexMatchTimeoutException) { return null; } - if (!block.Success) return null; - - var yaml = block.Groups[1].Value; - - string? name = null, description = null, license = null, compatibility = null, allowedTools = null; - foreach (Match m in TopLevelKeyValue.Matches(yaml)) - { - var value = ExtractValue(m); - switch (m.Groups[1].Value.ToLowerInvariant()) - { - case "name": name = value; break; - case "description": description = value; break; - case "license": license = value; break; - case "compatibility": compatibility = value; break; - case "allowed-tools": allowedTools = value; break; - } - } - - Dictionary? metadata = null; - var metadataMatch = MetadataBlock.Match(yaml); - if (metadataMatch.Success) - { - metadata = new Dictionary(StringComparer.Ordinal); - foreach (Match m in IndentedKeyValue.Matches(metadataMatch.Groups[1].Value)) - metadata[m.Groups[1].Value] = ExtractValue(m); - } - - if (string.IsNullOrEmpty(name) && string.IsNullOrEmpty(description) && - license is null && compatibility is null && allowedTools is null && metadata is null) - return null; - - return new SkillFrontmatter( - name ?? string.Empty, - description ?? string.Empty, - license, - compatibility, - allowedTools, - metadata); - } - - private static string ExtractValue(Match m) => - (m.Groups[2].Success ? m.Groups[2].Value - : m.Groups[3].Success ? m.Groups[3].Value - : m.Groups[4].Success ? m.Groups[4].Value - : string.Empty).Trim(); - - /// - /// Validates a skill name: 1-64 characters, lowercase letters/numbers/hyphens only, - /// no leading/trailing/consecutive hyphens. - /// - public static bool ValidateName(string? name, out string? reason) - { - if (string.IsNullOrWhiteSpace(name)) - { - reason = "Skill name is required."; - return false; - } - if (name.Length > MaxNameLength) - { - reason = $"Skill name must be {MaxNameLength} characters or fewer."; - return false; - } - if (!ValidNameRegex.IsMatch(name)) - { - reason = "Skill name must use only lowercase letters, numbers, and hyphens, and must not start or end with a hyphen or contain consecutive hyphens."; - return false; - } - reason = null; - return true; - } - - /// Validates a skill description: required, non-empty, 1-1024 characters. - public static bool ValidateDescription(string? description, out string? reason) - { - if (string.IsNullOrWhiteSpace(description)) - { - reason = "Skill description is required."; - return false; - } - if (description.Length > MaxDescriptionLength) - { - reason = $"Skill description must be {MaxDescriptionLength} characters or fewer."; - return false; - } - reason = null; - return true; - } - - /// Validates the optional compatibility field: at most 500 characters. - public static bool ValidateCompatibility(string? compatibility, out string? reason) - { - if (compatibility?.Length > MaxCompatibilityLength) - { - reason = $"Skill compatibility must be {MaxCompatibilityLength} characters or fewer."; - return false; - } - reason = null; - return true; - } - - /// - /// Full conformance check for a parsed frontmatter against a specific skill directory name — - /// the same checks fuseraft's orchestration skills provider applies. Returns every violation - /// found (empty list means fully compliant). - /// - public static IReadOnlyList Validate(SkillFrontmatter? frontmatter, string directoryName) - { - var violations = new List(); - - if (frontmatter is null) - { - violations.Add("No YAML frontmatter block found (SKILL.md must start with a '---' delimited block)."); - return violations; - } - - if (!ValidateName(frontmatter.Name, out var nameReason)) - { - violations.Add(nameReason!); - } - else if (!string.Equals(frontmatter.Name, directoryName, StringComparison.Ordinal)) - { - violations.Add($"'name: {frontmatter.Name}' does not match parent directory name '{directoryName}'."); - } - - if (!ValidateDescription(frontmatter.Description, out var descReason)) - violations.Add(descReason!); - - if (!ValidateCompatibility(frontmatter.Compatibility, out var compatReason)) - violations.Add(compatReason!); - - return violations; - } - - /// - /// Converts an arbitrary string into a spec-valid slug: lowercase, non-alphanumeric runs - /// collapsed to single hyphens, no leading/trailing hyphens. Used to derive a directory - /// name (and, after normalization, the name: field) from a skill's raw title. - /// - public static string ToSlug(string name) => - SlugSanitizer.Replace((name ?? string.Empty).ToLowerInvariant().Trim(), "-").Trim('-'); - - /// - /// Rewrites the name: line of a SKILL.md's frontmatter to , - /// leaving the rest of the file untouched. Used when installing or curating a skill whose - /// original name: field doesn't match the slug it will be installed under — without - /// this, the file on disk and its own directory name would disagree, which fuseraft's - /// orchestration skills provider treats as an invalid skill and silently drops. - /// Appends a name: line to the frontmatter block if one was missing entirely. - /// - public static string WithCanonicalName(string content, string slug) - { - var trimmed = TrimBom(content); - Match block; - try { block = FrontmatterBlock.Match(trimmed); } - catch (RegexMatchTimeoutException) { return content; } - if (!block.Success) return content; - - var yaml = block.Groups[1].Value; - var nameLine = $"name: {slug}"; - - string newYaml; - var nameMatch = TopLevelKeyValue.Matches(yaml) - .Cast() - .FirstOrDefault(m => string.Equals(m.Groups[1].Value, "name", StringComparison.OrdinalIgnoreCase)); - - if (nameMatch is not null) - { - newYaml = yaml[..nameMatch.Index] + nameLine + yaml[(nameMatch.Index + nameMatch.Length)..]; - } - else - { - // The captured yaml group starts right after "---" and before its own trailing - // newline (the '$' anchor is zero-width), so it always begins with '\n' — restore - // that leading newline here to keep "name:" on its own line after "---". - newYaml = "\n" + nameLine + "\n" + yaml.TrimStart('\n'); - } - - return trimmed[..block.Groups[1].Index] + newYaml + trimmed[(block.Groups[1].Index + block.Groups[1].Length)..]; - } - - /// Strips a leading UTF-8 BOM, which some editors prepend and which would - /// otherwise prevent the frontmatter block regex from matching at position 0. - private static string TrimBom(string content) => - content.Length > 0 && content[0] == '\uFEFF' ? content[1..] : content; -} diff --git a/src/Core/Skills/SkillPathGuard.cs b/src/Core/Skills/SkillPathGuard.cs deleted file mode 100644 index 4fe334a9..00000000 --- a/src/Core/Skills/SkillPathGuard.cs +++ /dev/null @@ -1,71 +0,0 @@ -namespace fuseraft.Core.Skills; - -/// -/// Path-containment and symlink-escape checks for resolving a model-supplied relative path -/// against a trusted skill directory root. -/// -/// -/// only normalizes a path lexically (collapsing -/// .. segments) — it does not resolve symbolic links. A lexical containment check alone -/// (resolved.StartsWith(skillRoot)) is therefore not sufficient: a symlink planted -/// anywhere inside a skill directory (e.g. references symlinked to /etc, or a -/// single file symlinked to ~/.ssh/id_rsa) would pass that check while actually reading -/// or executing a file outside the skill. This mirrors the symlink-escape protection in -/// Microsoft's AgentFileSkillsSource, which fuseraft's orchestration skills provider is -/// built on. -/// -/// -public static class SkillPathGuard -{ - /// - /// Resolves against and - /// confirms the result stays inside the root with no symlinked path segment along the way. - /// - public static bool TryResolveSafePath( - string skillRoot, - string relativePath, - out string fullPath, - out string? reason) - { - var root = Path.GetFullPath(skillRoot); - var rootWithSep = root.EndsWith(Path.DirectorySeparatorChar) ? root : root + Path.DirectorySeparatorChar; - - fullPath = Path.GetFullPath(Path.Combine(root, relativePath)); - if (!fullPath.StartsWith(rootWithSep, StringComparison.Ordinal)) - { - reason = "path is outside the skill directory."; - return false; - } - - var relative = Path.GetRelativePath(root, fullPath); - var current = root; - foreach (var segment in relative.Split(Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries)) - { - current = Path.Combine(current, segment); - if (!File.Exists(current) && !Directory.Exists(current)) - continue; // let the caller's own not-found handling report this - - FileAttributes attrs; - try { attrs = File.GetAttributes(current); } - catch (IOException) { continue; } - catch (UnauthorizedAccessException) { continue; } - - if (attrs.HasFlag(FileAttributes.ReparsePoint)) - { - reason = $"'{segment}' is a symlink; skill paths may not traverse symlinks."; - return false; - } - } - - reason = null; - return true; - } - - /// True when exists and is a symlink/reparse point. - public static bool IsReparsePoint(string path) - { - try { return File.GetAttributes(path).HasFlag(FileAttributes.ReparsePoint); } - catch (IOException) { return false; } - catch (UnauthorizedAccessException) { return false; } - } -} diff --git a/src/Infrastructure/Plugins/SkillsPlugin.cs b/src/Infrastructure/Plugins/SkillsPlugin.cs deleted file mode 100644 index 100dad91..00000000 --- a/src/Infrastructure/Plugins/SkillsPlugin.cs +++ /dev/null @@ -1,148 +0,0 @@ -using System.ComponentModel; -using System.Diagnostics; -using System.Runtime.InteropServices; -using fuseraft.Core.Skills; - -namespace fuseraft.Infrastructure.Plugins; - -/// -/// Exposes skills from the library as callable tools. -/// -/// -/// Skills follow the progressive-disclosure pattern: at session start the REPL -/// injects a catalog of skill names and descriptions into the system prompt so the -/// model knows what is available. When the model decides to apply a skill it calls -/// load_skill to retrieve the full step-by-step SKILL.md body, then follows -/// those instructions using its other tools. read_skill_resource is available -/// for skills that ship supplementary reference files (e.g. under references/) -/// alongside their SKILL.md, and run_skill_script for skills that ship -/// executable scripts. -/// -/// -public sealed class SkillsPlugin -{ - // slug → directory that contains SKILL.md (and any scripts) - private readonly IReadOnlyDictionary _skillDirs; - - public int Count => _skillDirs.Count; - - public IEnumerable Slugs => _skillDirs.Keys; - - public bool HasSkill(string slug) => _skillDirs.ContainsKey(slug); - - public SkillsPlugin(IReadOnlyDictionary skillDirs) - { - _skillDirs = skillDirs; - } - - [Description("Load full instructions for a skill by slug.")] - public async Task LoadSkillAsync( - [Description("Skill slug, e.g. 'fetch-remote-api'.")] string name, - CancellationToken cancellationToken = default) - { - if (!_skillDirs.TryGetValue(name, out var dir)) - { - var known = string.Join(", ", _skillDirs.Keys.Take(10)); - return PluginResult.NotFound($"No skill '{name}'. Available: {known}"); - } - - var skillPath = Path.Combine(dir, "SKILL.md"); - if (!File.Exists(skillPath)) - return PluginResult.Error($"SKILL.md missing for '{name}'."); - - try - { - return await File.ReadAllTextAsync(skillPath, cancellationToken); - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - return PluginResult.Error($"Could not read skill '{name}': {ex.Message}"); - } - } - - [Description("Read a reference/resource file bundled with a skill, e.g. a file under 'references/'.")] - public async Task ReadSkillResourceAsync( - [Description("Skill slug.")] string skill, - [Description("Resource path relative to the skill directory, e.g. 'references/style-guide.md'.")] string resourcePath, - CancellationToken cancellationToken = default) - { - if (!_skillDirs.TryGetValue(skill, out var dir)) - return PluginResult.NotFound($"No skill '{skill}'."); - - if (string.IsNullOrWhiteSpace(resourcePath)) - return PluginResult.Error("Resource path must not be empty."); - - // Resolve against the skill directory and confirm the result stays inside it, with no - // symlinked path segment along the way — resourcePath comes from the model, so an - // absolute path, a "../" sequence, or a symlink planted in the skill directory must not - // be able to escape to arbitrary files on disk. - if (!SkillPathGuard.TryResolveSafePath(dir, resourcePath, out var fullPath, out var reason)) - return PluginResult.Error($"'{resourcePath}' {reason}"); - - if (!File.Exists(fullPath)) - return PluginResult.NotFound($"Resource '{resourcePath}' not found in skill '{skill}'."); - - try - { - return await File.ReadAllTextAsync(fullPath, cancellationToken); - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - return PluginResult.Error($"Could not read resource '{resourcePath}': {ex.Message}"); - } - } - - [Description("Run a script bundled with a skill.")] - public async Task RunSkillScriptAsync( - [Description("Skill slug.")] string skill, - [Description("Script filename inside the skill directory, e.g. 'transform.py'.")] string script, - [Description("Space-separated arguments to pass to the script.")] string args = "", - CancellationToken cancellationToken = default) - { - if (!_skillDirs.TryGetValue(skill, out var dir)) - return PluginResult.NotFound($"No skill '{skill}'."); - - // Resolve against the skill directory and confirm the result stays inside it — same - // containment/symlink check as ReadSkillResourceAsync, since 'script' comes from the model. - if (!SkillPathGuard.TryResolveSafePath(dir, script, out var scriptPath, out var reason)) - return PluginResult.Error($"'{script}' {reason}"); - - if (!File.Exists(scriptPath)) - return PluginResult.NotFound($"Script '{script}' not found in skill '{skill}'."); - - var ext = Path.GetExtension(scriptPath).ToLowerInvariant(); - var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); - var program = ext switch - { - ".py" => isWindows ? "python" : "python3", - ".sh" => "bash", - ".js" => "node", - _ => null, - }; - if (program is null) - return PluginResult.Error($"No runner registered for '{ext}' scripts."); - - var psi = new ProcessStartInfo - { - FileName = program, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - }; - psi.ArgumentList.Add(scriptPath); - foreach (var a in args.Split(' ', StringSplitOptions.RemoveEmptyEntries)) - psi.ArgumentList.Add(a); - - using var proc = Process.Start(psi) - ?? throw new InvalidOperationException($"Failed to start {program}"); - - var stdoutTask = proc.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = proc.StandardError.ReadToEndAsync(cancellationToken); - await Task.WhenAll(stdoutTask, stderrTask); - await proc.WaitForExitAsync(cancellationToken); - - var stdout = await stdoutTask; - var stderr = await stderrTask; - return string.IsNullOrWhiteSpace(stderr) ? stdout : $"{stdout}\nstderr: {stderr}"; - } -} diff --git a/src/Orchestration/Skills/SkillCurator.cs b/src/Orchestration/Skills/SkillCurator.cs index f8a9330e..78790562 100644 --- a/src/Orchestration/Skills/SkillCurator.cs +++ b/src/Orchestration/Skills/SkillCurator.cs @@ -2,6 +2,7 @@ using System.Text.Json; using System.Text.Json.Serialization; using System.Text.RegularExpressions; +using Microsoft.Agents.AI; using Microsoft.Extensions.AI; using Microsoft.Extensions.Logging; using fuseraft.Core; @@ -165,58 +166,38 @@ public async Task RunAsync( } var skillContent = match.Groups[1].Value.Trim(); - var frontmatter = SkillFrontmatterSpec.TryParse(skillContent); - if (string.IsNullOrWhiteSpace(frontmatter?.Name)) - { - const string noNameReason = "SKILL block is missing the 'name:' frontmatter field."; - logger.LogWarning( - "Skill curation failed — session={Session} reason={Reason}", - checkpoint.SessionId, noNameReason); - var failed = new SkillCurationResult( - SkillCurationOutcome.Failed, - FailureReason: noNameReason, - TurnsDigested: digestTurns, - Model: modelId); - await AppendCurationLogAsync(checkpoint.SessionId, failed, source, ct); - return failed; - } - - if (!SkillFrontmatterSpec.ValidateDescription(frontmatter.Description, out var descReason)) + // Curation writes a brand-new file, so — unlike 'skills add' — there is no existing + // directory name to reconcile a sloppy title against. The system prompt above already + // instructs the model to emit a ready-made kebab-case slug; strict validation here + // (rather than silently slugifying whatever it produced) catches the rare case where it + // didn't, instead of writing something that would look fine here but be silently dropped + // by fuseraft's orchestration skills provider. AgentSkillFrontmatter's own constructor + // is the sole authority on whether the raw name/description/compatibility are valid. + var rawName = FrontmatterFieldReader.ExtractField(skillContent, "name"); + var rawDescription = FrontmatterFieldReader.ExtractField(skillContent, "description"); + var rawCompatibility = FrontmatterFieldReader.ExtractField(skillContent, "compatibility"); + + AgentSkillFrontmatter frontmatter; + try { - logger.LogWarning( - "Skill curation failed — session={Session} reason={Reason}", - checkpoint.SessionId, descReason); - var failed = new SkillCurationResult( - SkillCurationOutcome.Failed, - FailureReason: descReason, - TurnsDigested: digestTurns, - Model: modelId); - await AppendCurationLogAsync(checkpoint.SessionId, failed, source, ct); - return failed; + frontmatter = new AgentSkillFrontmatter(rawName ?? string.Empty, rawDescription ?? string.Empty, rawCompatibility); } - - var slug = SkillFrontmatterSpec.ToSlug(frontmatter.Name); - if (!SkillFrontmatterSpec.ValidateName(slug, out var slugReason)) + catch (ArgumentException ex) { - var badSlugReason = $"Derived slug '{slug}' from name '{frontmatter.Name}' is invalid: {slugReason}"; logger.LogWarning( "Skill curation failed — session={Session} reason={Reason}", - checkpoint.SessionId, badSlugReason); + checkpoint.SessionId, ex.Message); var failed = new SkillCurationResult( SkillCurationOutcome.Failed, - FailureReason: badSlugReason, + FailureReason: ex.Message, TurnsDigested: digestTurns, Model: modelId); await AppendCurationLogAsync(checkpoint.SessionId, failed, source, ct); return failed; } - // Guarantee the written file's 'name:' matches the directory it's written under — the - // LLM's raw name may need slugifying (spaces, uppercase, ...), and without this the file - // and its own directory would disagree, which fuseraft's orchestration skills provider - // treats as invalid and silently drops. - skillContent = SkillFrontmatterSpec.WithCanonicalName(skillContent, slug); + var slug = frontmatter.Name; try { diff --git a/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs b/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs index cd78eefb..ff1b8001 100644 --- a/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs +++ b/tests/FuseraftCli.Tests/ReplSkillsLoaderTests.cs @@ -1,267 +1,133 @@ +using Microsoft.Extensions.AI; +using Microsoft.Extensions.Logging.Abstractions; using fuseraft.Cli.Commands.Repl; namespace FuseraftCli.Tests; /// -/// Tests for : directory scanning, frontmatter parsing, -/// catalog generation, and resilience to bad/missing inputs. +/// Tests for : the thin REPL-side wiring over +/// Microsoft.Agents.AI's AgentFileSkillsSource/AgentSkillsProvider. /// -/// All tests use an isolated temp directory; no real skill library is touched. +/// +/// These tests deliberately do not re-verify frontmatter validation rules (kebab-case format, +/// length limits, name-matches-directory, ...) — that's Microsoft's own, separately-tested +/// behavior. What's fuseraft-specific and worth covering here is the wiring itself: that +/// discovery results, catalog instructions, and tools all come back consistently, and that +/// dedup/precedence across multiple search directories works as the REPL depends on. +/// /// public sealed class ReplSkillsLoaderTests : IDisposable { private readonly string _root; + private static readonly IChatClient StubClient = new NonInvocableStubChatClient(); public ReplSkillsLoaderTests() { - _root = Path.Combine(Path.GetTempPath(), "fuseraft_loader_tests_" + Guid.NewGuid().ToString("N")[..8]); + _root = Path.Combine(Path.GetTempPath(), "fuseraft_repl_loader_tests_" + Guid.NewGuid().ToString("N")[..8]); Directory.CreateDirectory(_root); } public void Dispose() => Directory.Delete(_root, recursive: true); - // ── helpers ─────────────────────────────────────────────────────────────── - - /// Creates a skill dir with a SKILL.md at _root/slug/SKILL.md. - private string WriteSkill(string slug, string content) + private string WriteSkill(string relativeDir, string name, string description, string body = "## Steps\n1. Do it.") { - var dir = Path.Combine(_root, slug); + var dir = Path.Combine(_root, relativeDir); Directory.CreateDirectory(dir); - File.WriteAllText(Path.Combine(dir, "SKILL.md"), content); + File.WriteAllText(Path.Combine(dir, "SKILL.md"), + $"---\nname: {name}\ndescription: \"{description}\"\n---\n\n{body}"); return dir; } - private string ValidSkillMd(string name, string description, string body = "## Steps\n1. Do it.") - => $"---\nname: {name}\ndescription: \"{description}\"\n---\n\n{body}"; - - // ── ParseSkillDescription ───────────────────────────────────────────────── + private static Task Build(params string[] searchDirs) => + ReplSkillsLoader.BuildAsync(StubClient, NullLoggerFactory.Instance, searchDirs, CancellationToken.None); [Fact] - public void ParseSkillDescription_WellFormedFrontmatter_ReturnsDescription() + public async Task BuildAsync_NoSearchDirs_ReturnsEmpty() { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\nname: my-skill\ndescription: \"Use when doing X.\"\n---\n\n# Body"); + var result = await Build(); - var desc = ReplSkillsLoader.ParseSkillDescription(path); - Assert.Equal("Use when doing X.", desc); + Assert.Empty(result.Skills); + Assert.Null(result.CatalogInstructions); + Assert.Empty(result.Tools); } [Fact] - public void ParseSkillDescription_SingleQuotedValue_ReturnsUnquotedDescription() + public async Task BuildAsync_SearchDirDoesNotExist_ReturnsEmpty() { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\ndescription: 'Use when doing Y.'\n---"); + var result = await Build(Path.Combine(_root, "nonexistent")); - var desc = ReplSkillsLoader.ParseSkillDescription(path); - Assert.Equal("Use when doing Y.", desc); + Assert.Empty(result.Skills); + Assert.Null(result.CatalogInstructions); } [Fact] - public void ParseSkillDescription_UnquotedValue_ReturnsTrimmedDescription() + public async Task BuildAsync_OneValidSkill_ReturnsSkillCatalogAndTools() { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\ndescription: Use when doing Z.\n---"); - - var desc = ReplSkillsLoader.ParseSkillDescription(path); - Assert.Equal("Use when doing Z.", desc); - } + WriteSkill("fetch-api", "fetch-api", "Use when fetching REST data."); - [Fact] - public void ParseSkillDescription_DescriptionWithColons_ReturnsFullValue() - { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\ndescription: \"Use when: A, B, or C.\"\n---"); + var result = await Build(_root); - var desc = ReplSkillsLoader.ParseSkillDescription(path); - Assert.Equal("Use when: A, B, or C.", desc); + Assert.Single(result.Skills); + Assert.Equal("fetch-api", result.Skills[0].Frontmatter.Name); + Assert.NotNull(result.CatalogInstructions); + Assert.Contains("fetch-api", result.CatalogInstructions); + Assert.Contains("Use when fetching REST data.", result.CatalogInstructions); } [Fact] - public void ParseSkillDescription_EmptyValue_ReturnsNull() + public async Task BuildAsync_ValidSkill_ExposesLoadReadRunSkillTools() { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\ndescription: \"\"\n---"); + WriteSkill("my-skill", "my-skill", "A skill."); - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); - } + var result = await Build(_root); - [Fact] - public void ParseSkillDescription_WhitespaceOnlyValue_ReturnsNull() - { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\ndescription: \n---"); - - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); + var toolNames = result.Tools.Select(t => t.Name).ToList(); + Assert.Contains("load_skill", toolNames); + Assert.Contains("read_skill_resource", toolNames); + Assert.Contains("run_skill_script", toolNames); } [Fact] - public void ParseSkillDescription_NoDescriptionField_ReturnsNull() + public async Task BuildAsync_LoadSkillTool_ReturnsFullContent() { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\nname: my-skill\n---\n\n# Body"); - - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); - } - - [Fact] - public void ParseSkillDescription_NoFrontmatter_ReturnsNull() - { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "# No frontmatter here\n\nJust body text."); - - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); - } - - [Fact] - public void ParseSkillDescription_UnclosedFrontmatter_ReturnsNull() - { - // Opening --- but no closing ---; reads to EOF without finding the field. - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, "---\nname: skill\n\nNo closing delimiter"); - - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); - } - - [Fact] - public void ParseSkillDescription_EmptyFile_ReturnsNull() - { - var path = Path.Combine(_root, "skill.md"); - File.WriteAllText(path, ""); - - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); - } - - [Fact] - public void ParseSkillDescription_FileDoesNotExist_ReturnsNull() - { - var path = Path.Combine(_root, "nonexistent.md"); - Assert.Null(ReplSkillsLoader.ParseSkillDescription(path)); - } - - // ── BuildSkills — no skills ─────────────────────────────────────────────── - - [Fact] - public void BuildSkills_NoSearchDirs_ReturnsNull() - { - var (plugin, catalog) = ReplSkillsLoader.BuildSkills(Array.Empty()); - Assert.Null(plugin); - Assert.Null(catalog); - } - - [Fact] - public void BuildSkills_SearchDirDoesNotExist_ReturnsNull() - { - var (plugin, catalog) = ReplSkillsLoader.BuildSkills([Path.Combine(_root, "nonexistent")]); - Assert.Null(plugin); - Assert.Null(catalog); - } - - [Fact] - public void BuildSkills_SearchDirExistsButEmpty_ReturnsNull() - { - var (plugin, catalog) = ReplSkillsLoader.BuildSkills([_root]); - Assert.Null(plugin); - Assert.Null(catalog); - } - - [Fact] - public void BuildSkills_DirHasNoSkillMdFiles_ReturnsNull() - { - // A file called something else — should be ignored. - File.WriteAllText(Path.Combine(_root, "README.md"), "not a skill"); - var (plugin, catalog) = ReplSkillsLoader.BuildSkills([_root]); - Assert.Null(plugin); - Assert.Null(catalog); - } - - // ── BuildSkills — valid skills ──────────────────────────────────────────── - - [Fact] - public void BuildSkills_OneValidSkill_ReturnsPluginAndCatalog() - { - WriteSkill("fetch-api", ValidSkillMd("fetch-api", "Use when fetching REST data.")); - - var (plugin, catalog) = ReplSkillsLoader.BuildSkills([_root]); - - Assert.NotNull(plugin); - Assert.NotNull(catalog); - Assert.Equal(1, plugin!.Count); - } - - [Fact] - public void BuildSkills_CatalogContainsSlugAndDescription() - { - WriteSkill("fetch-api", ValidSkillMd("fetch-api", "Use when fetching REST data.")); - - var (_, catalog) = ReplSkillsLoader.BuildSkills([_root]); - - Assert.Contains("fetch-api", catalog!); - Assert.Contains("Use when fetching REST data.", catalog); - } - - [Fact] - public void BuildSkills_CatalogContainsLoadSkillInstruction() - { - WriteSkill("my-skill", ValidSkillMd("my-skill", "A skill.")); - - var (_, catalog) = ReplSkillsLoader.BuildSkills([_root]); - - Assert.Contains("load_skill", catalog!); - } - - [Fact] - public void BuildSkills_MultipleSkills_AllAppearInCatalog() - { - WriteSkill("alpha", ValidSkillMd("alpha", "First skill.")); - WriteSkill("beta", ValidSkillMd("beta", "Second skill.")); - - var (plugin, catalog) = ReplSkillsLoader.BuildSkills([_root]); + WriteSkill("my-skill", "my-skill", "A skill.", body: "## Do the thing\nStep one."); - Assert.Equal(2, plugin!.Count); - Assert.Contains("alpha", catalog!); - Assert.Contains("beta", catalog); - } + var result = await Build(_root); + var loadSkill = result.Tools.Single(t => t.Name == "load_skill"); - [Fact] - public void BuildSkills_CatalogSlugsAreSorted() - { - WriteSkill("zebra", ValidSkillMd("zebra", "Z skill.")); - WriteSkill("alpha", ValidSkillMd("alpha", "A skill.")); + var content = await loadSkill.InvokeAsync(new AIFunctionArguments { ["skillName"] = "my-skill" }); - var (_, catalog) = ReplSkillsLoader.BuildSkills([_root]); - - var alphaPos = catalog!.IndexOf("alpha", StringComparison.Ordinal); - var zebraPos = catalog.IndexOf("zebra", StringComparison.Ordinal); - Assert.True(alphaPos < zebraPos, "Catalog should list skills in alphabetical order"); + Assert.Contains("Do the thing", content?.ToString()); } [Fact] - public void BuildSkills_SkillWithNoDescription_SlugAppearsWithoutTrailingColon() + public async Task BuildAsync_NameDoesNotMatchDirectory_SkillIsSilentlyExcluded() { - // Skill with no description field — just a bare slug in the catalog. - WriteSkill("bare-skill", "# No frontmatter here"); + // AgentFileSkillsSource's own validation, not fuseraft's — covered here only to confirm + // the wiring surfaces that behavior rather than working around it. + WriteSkill("mismatched-dir", "totally-different-name", "A description."); - var (plugin, catalog) = ReplSkillsLoader.BuildSkills([_root]); + var result = await Build(_root); - Assert.NotNull(plugin); - Assert.Contains("bare-skill", catalog!); - Assert.DoesNotContain("bare-skill:", catalog); // no trailing colon + Assert.Empty(result.Skills); + Assert.Null(result.CatalogInstructions); } [Fact] - public void BuildSkills_SkillWithEmptyDescription_SlugAppearsWithoutTrailingColon() + public async Task BuildAsync_MultipleValidSkills_AllDiscovered() { - WriteSkill("empty-desc", "---\ndescription: \"\"\n---\n# Body"); + WriteSkill("alpha", "alpha", "First skill."); + WriteSkill("beta", "beta", "Second skill."); - var (_, catalog) = ReplSkillsLoader.BuildSkills([_root]); + var result = await Build(_root); - Assert.DoesNotContain("empty-desc:", catalog!); + Assert.Equal(2, result.Skills.Count); + Assert.Contains(result.Skills, s => s.Frontmatter.Name == "alpha"); + Assert.Contains(result.Skills, s => s.Frontmatter.Name == "beta"); } - // ── BuildSkills — priority and deduplication ────────────────────────────── - [Fact] - public void BuildSkills_DuplicateSlugAcrossDirs_FirstDirWins() + public async Task BuildAsync_DuplicateSlugAcrossSearchDirs_DeduplicatedAndFirstDirWins() { var dir1 = Path.Combine(_root, "priority1"); var dir2 = Path.Combine(_root, "priority2"); @@ -272,188 +138,39 @@ public void BuildSkills_DuplicateSlugAcrossDirs_FirstDirWins() var skill2 = Path.Combine(dir2, "my-skill"); Directory.CreateDirectory(skill1); Directory.CreateDirectory(skill2); - File.WriteAllText(Path.Combine(skill1, "SKILL.md"), "---\ndescription: \"From dir1\"\n---"); - File.WriteAllText(Path.Combine(skill2, "SKILL.md"), "---\ndescription: \"From dir2\"\n---"); + File.WriteAllText(Path.Combine(skill1, "SKILL.md"), "---\nname: my-skill\ndescription: \"From dir1\"\n---"); + File.WriteAllText(Path.Combine(skill2, "SKILL.md"), "---\nname: my-skill\ndescription: \"From dir2\"\n---"); - var (_, catalog) = ReplSkillsLoader.BuildSkills([dir1, dir2]); + var result = await Build(dir1, dir2); - Assert.Contains("From dir1", catalog!); - Assert.DoesNotContain("From dir2", catalog); + // The banner count (result.Skills) must agree with what the catalog actually advertises — + // both must be deduplicated by name, not just the catalog. + Assert.Single(result.Skills); + Assert.Equal("From dir1", result.Skills[0].Frontmatter.Description); + Assert.Contains("From dir1", result.CatalogInstructions!); + Assert.DoesNotContain("From dir2", result.CatalogInstructions!); } - // ── BuildSkills — resilience ────────────────────────────────────────────── - [Fact] - public void BuildSkills_SkillMdWithGarbageContent_DoesNotThrow() + public async Task BuildAsync_DirHasNoSkillMdFiles_ReturnsEmpty() { - // Completely invalid content — should be indexed with a null description. - WriteSkill("garbage", "\x00\x01\x02 not UTF-8 friendly binary content \xff\xfe"); - - var ex = Record.Exception(() => ReplSkillsLoader.BuildSkills([_root])); - Assert.Null(ex); - } - - [Fact] - public void BuildSkills_MixOfValidAndInvalidSkills_ValidOnesStillLoaded() - { - WriteSkill("good", ValidSkillMd("good", "A well-formed skill.")); - WriteSkill("badfile", "---\n: invalid yaml :\n---"); - - var (plugin, _) = ReplSkillsLoader.BuildSkills([_root]); - - Assert.NotNull(plugin); - Assert.Equal(2, plugin!.Count); // both dirs indexed; bad frontmatter just gives null desc - } - - [Fact] - public void BuildSkills_EmptySkillMd_DoesNotThrow() - { - WriteSkill("empty", ""); - - var ex = Record.Exception(() => ReplSkillsLoader.BuildSkills([_root])); - Assert.Null(ex); - } - - [Fact] - public void BuildSkills_SkillMdIsDirectory_DoesNotThrow() - { - // Edge case: a path called "SKILL.md" that is actually a directory. - var slugDir = Path.Combine(_root, "weird-skill"); - var fakeMd = Path.Combine(slugDir, "SKILL.md"); - Directory.CreateDirectory(fakeMd); // SKILL.md is a directory, not a file - - var ex = Record.Exception(() => ReplSkillsLoader.BuildSkills([_root])); - Assert.Null(ex); - } - - // ── BuildSkillsDetailed — spec conformance (parity with orchestration) ──── - - [Fact] - public void BuildSkillsDetailed_NameDoesNotMatchDirectory_SkipsWithWarning() - { - WriteSkill("mismatched-dir", ValidSkillMd("totally-different-name", "A description.")); - - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); - - Assert.Null(result.Plugin); - Assert.Contains(result.Warnings, w => w.Contains("does not match") && w.Contains("mismatched-dir")); - } - - [Fact] - public void BuildSkillsDetailed_DeclaredNameInvalidFormat_SkipsWithWarning() - { - WriteSkill("Bad-Name", ValidSkillMd("Bad-Name", "A description.")); // uppercase not allowed - - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); - - Assert.Null(result.Plugin); - Assert.Single(result.Warnings); - } - - [Fact] - public void BuildSkillsDetailed_DeclaredDescriptionTooLong_SkipsWithWarning() - { - var longDescription = new string('a', 1025); - WriteSkill("my-skill", ValidSkillMd("my-skill", longDescription)); - - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); - - Assert.Null(result.Plugin); - Assert.Contains(result.Warnings, w => w.Contains("1024")); - } - - [Fact] - public void BuildSkillsDetailed_NoNameFieldAtAll_LoadsLeniently_NoWarning() - { - // Directory-name-only skills (no 'name:' field) remain a supported, warning-free - // REPL convenience even though orchestration requires a declared, matching name. - WriteSkill("my-skill", "---\ndescription: \"A description.\"\n---\n\nBody."); - - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); - - Assert.NotNull(result.Plugin); - Assert.Equal(1, result.Plugin!.Count); - Assert.Empty(result.Warnings); - } - - [Fact] - public void BuildSkillsDetailed_ValidNameAndDirectoryMatch_NoWarning() - { - WriteSkill("my-skill", ValidSkillMd("my-skill", "A description.")); - - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); - - Assert.NotNull(result.Plugin); - Assert.Empty(result.Warnings); - } - - [Fact] - public void BuildSkills_CompatibilityField_AppearsInCatalog() - { - var dir = Path.Combine(_root, "my-skill"); - Directory.CreateDirectory(dir); - File.WriteAllText(Path.Combine(dir, "SKILL.md"), - "---\nname: my-skill\ndescription: \"A description.\"\ncompatibility: \"Requires docker\"\n---\n\nBody."); - - var (_, catalog) = ReplSkillsLoader.BuildSkills([_root]); - - Assert.Contains("Requires docker", catalog!); - } - - [Fact] - public void BuildSkillsDetailed_DeclaredCompatibilityTooLong_SkipsWithWarning() - { - var dir = Path.Combine(_root, "my-skill"); - Directory.CreateDirectory(dir); - var longCompat = new string('a', 501); - File.WriteAllText(Path.Combine(dir, "SKILL.md"), - $"---\nname: my-skill\ndescription: \"A description.\"\ncompatibility: \"{longCompat}\"\n---\n\nBody."); + File.WriteAllText(Path.Combine(_root, "README.md"), "not a skill"); - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + var result = await Build(_root); - Assert.Null(result.Plugin); - Assert.Contains(result.Warnings, w => w.Contains("500")); + Assert.Empty(result.Skills); } - [Fact] - public void BuildSkillsDetailed_NestedVendorNamespace_TwoLevelsDeep_IsDiscovered() + private sealed class NonInvocableStubChatClient : IChatClient { - // Matches orchestration's AgentFileSkillsSource search depth (root/vendor/skill/SKILL.md). - WriteSkill(Path.Combine("vendor", "my-skill"), ValidSkillMd("my-skill", "A description.")); + public Task GetResponseAsync(IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) => + throw new NotSupportedException("Skill discovery should never actually invoke the chat client."); - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); + public IAsyncEnumerable GetStreamingResponseAsync(IEnumerable messages, ChatOptions? options = null, CancellationToken cancellationToken = default) => + throw new NotSupportedException("Skill discovery should never actually invoke the chat client."); - Assert.NotNull(result.Plugin); - Assert.True(result.Plugin!.HasSkill("my-skill")); - } + public object? GetService(Type serviceType, object? serviceKey = null) => null; - [Fact] - public void BuildSkillsDetailed_SymlinkedSkillDirectory_IsNotFollowed() - { - var realSkillRoot = Path.Combine(Path.GetTempPath(), "fuseraft_loader_tests_real_" + Guid.NewGuid().ToString("N")[..8]); - var realSkillDir = Path.Combine(realSkillRoot, "real-skill"); - Directory.CreateDirectory(realSkillDir); - File.WriteAllText(Path.Combine(realSkillDir, "SKILL.md"), ValidSkillMd("real-skill", "A description.")); - - try - { - var link = Path.Combine(_root, "linked-skill"); - try - { - Directory.CreateSymbolicLink(link, realSkillDir); - } - catch (Exception) - { - return; // environment doesn't allow symlinks — skip - } - - var result = ReplSkillsLoader.BuildSkillsDetailed([_root]); - - Assert.Null(result.Plugin); // the only skill lives behind a symlink, which must not be followed - } - finally - { - Directory.Delete(realSkillRoot, recursive: true); - } + public void Dispose() { } } } diff --git a/tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs b/tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs deleted file mode 100644 index b00b074e..00000000 --- a/tests/FuseraftCli.Tests/SkillFrontmatterSpecTests.cs +++ /dev/null @@ -1,274 +0,0 @@ -using fuseraft.Core.Skills; - -namespace FuseraftCli.Tests; - -/// -/// Tests for — the single parser/validator shared by the -/// REPL loader, orchestration-visible skills add/skills validate commands, and -/// skill curation, so all surfaces agree on what a spec-conformant SKILL.md looks like. -/// -public sealed class SkillFrontmatterSpecTests -{ - // ── TryParse ───────────────────────────────────────────────────────────── - - [Fact] - public void TryParse_NoFrontmatter_ReturnsNull() - { - Assert.Null(SkillFrontmatterSpec.TryParse("# Just a heading\n\nSome body text.")); - } - - [Fact] - public void TryParse_EmptyContent_ReturnsNull() - { - Assert.Null(SkillFrontmatterSpec.TryParse("")); - Assert.Null(SkillFrontmatterSpec.TryParse(null)); - } - - [Fact] - public void TryParse_UnclosedFrontmatter_ReturnsNull() - { - Assert.Null(SkillFrontmatterSpec.TryParse("---\nname: skill\n\nNo closing delimiter")); - } - - [Fact] - public void TryParse_MinimalValidFrontmatter_ExtractsNameAndDescription() - { - var fm = SkillFrontmatterSpec.TryParse("---\nname: pdf-processing\ndescription: Extract PDF text.\n---\n\nBody"); - Assert.NotNull(fm); - Assert.Equal("pdf-processing", fm!.Name); - Assert.Equal("Extract PDF text.", fm.Description); - } - - [Fact] - public void TryParse_DoubleQuotedDescriptionWithColon_PreservesColon() - { - var fm = SkillFrontmatterSpec.TryParse("---\ndescription: \"Use when: A, B, or C.\"\n---"); - Assert.Equal("Use when: A, B, or C.", fm!.Description); - } - - [Fact] - public void TryParse_SingleQuotedValue_Unquotes() - { - var fm = SkillFrontmatterSpec.TryParse("---\ndescription: 'Use when doing Y.'\n---"); - Assert.Equal("Use when doing Y.", fm!.Description); - } - - [Fact] - public void TryParse_OptionalFields_AllExtracted() - { - var content = """ - --- - name: pdf-processing - description: Extract PDF text, fill forms, merge files. - license: Apache-2.0 - compatibility: Requires Python 3.14+ and uv - allowed-tools: Bash(git:*) Bash(jq:*) Read - metadata: - author: example-org - version: "1.0" - --- - - Body. - """; - - var fm = SkillFrontmatterSpec.TryParse(content); - - Assert.NotNull(fm); - Assert.Equal("Apache-2.0", fm!.License); - Assert.Equal("Requires Python 3.14+ and uv", fm.Compatibility); - Assert.Equal("Bash(git:*) Bash(jq:*) Read", fm.AllowedTools); - Assert.NotNull(fm.Metadata); - Assert.Equal("example-org", fm.Metadata!["author"]); - Assert.Equal("1.0", fm.Metadata["version"]); - } - - [Fact] - public void TryParse_MetadataKeysDoNotLeakIntoTopLevelFields() - { - // A "name:" or "description:" line indented under metadata: must not be picked up - // as the top-level field — only unindented lines are top-level. - var content = """ - --- - name: real-skill - description: Real description. - metadata: - name: this-is-just-metadata - --- - """; - - var fm = SkillFrontmatterSpec.TryParse(content); - - Assert.Equal("real-skill", fm!.Name); - Assert.Equal("this-is-just-metadata", fm.Metadata!["name"]); - } - - [Fact] - public void TryParse_UnrecognizedTopLevelKeys_AreIgnoredWithoutError() - { - // Third-party skills sometimes add non-spec fields (e.g. "version:", "homepage:"). - var content = "---\nname: my-skill\ndescription: A skill.\nversion: 1.5.2\nhomepage: https://example.com\n---"; - var fm = SkillFrontmatterSpec.TryParse(content); - - Assert.Equal("my-skill", fm!.Name); - Assert.Equal("A skill.", fm.Description); - } - - [Fact] - public void TryParse_NoRecognizedFields_ReturnsNull() - { - Assert.Null(SkillFrontmatterSpec.TryParse("---\n: invalid yaml :\n---")); - } - - // ── ValidateName ───────────────────────────────────────────────────────── - - [Theory] - [InlineData("pdf-processing")] - [InlineData("data-analysis")] - [InlineData("a")] - [InlineData("a1-b2")] - public void ValidateName_ValidNames_Pass(string name) - { - Assert.True(SkillFrontmatterSpec.ValidateName(name, out var reason)); - Assert.Null(reason); - } - - [Fact] - public void ValidateName_Null_Fails() - { - Assert.False(SkillFrontmatterSpec.ValidateName(null, out var reason)); - Assert.Contains("required", reason); - } - - [Fact] - public void ValidateName_TooLong_Fails() - { - var name = new string('a', 65); - Assert.False(SkillFrontmatterSpec.ValidateName(name, out var reason)); - Assert.Contains("64", reason); - } - - [Theory] - [InlineData("PDF-Processing")] - [InlineData("-pdf")] - [InlineData("pdf-")] - [InlineData("pdf--processing")] - [InlineData("pdf_processing")] - [InlineData("pdf processing")] - public void ValidateName_InvalidFormats_Fail(string name) - { - Assert.False(SkillFrontmatterSpec.ValidateName(name, out var reason)); - Assert.NotNull(reason); - } - - // ── ValidateDescription ────────────────────────────────────────────────── - - [Fact] - public void ValidateDescription_Empty_Fails() - { - Assert.False(SkillFrontmatterSpec.ValidateDescription("", out var reason)); - Assert.Contains("required", reason); - } - - [Fact] - public void ValidateDescription_TooLong_Fails() - { - var desc = new string('a', 1025); - Assert.False(SkillFrontmatterSpec.ValidateDescription(desc, out var reason)); - Assert.Contains("1024", reason); - } - - [Fact] - public void ValidateDescription_ExactlyMaxLength_Passes() - { - var desc = new string('a', 1024); - Assert.True(SkillFrontmatterSpec.ValidateDescription(desc, out _)); - } - - // ── ValidateCompatibility ──────────────────────────────────────────────── - - [Fact] - public void ValidateCompatibility_Null_Passes() - { - Assert.True(SkillFrontmatterSpec.ValidateCompatibility(null, out var reason)); - Assert.Null(reason); - } - - [Fact] - public void ValidateCompatibility_TooLong_Fails() - { - Assert.False(SkillFrontmatterSpec.ValidateCompatibility(new string('a', 501), out var reason)); - Assert.Contains("500", reason); - } - - // ── Validate (full conformance) ────────────────────────────────────────── - - [Fact] - public void Validate_NullFrontmatter_ReportsMissingFrontmatter() - { - var violations = SkillFrontmatterSpec.Validate(null, "my-skill"); - Assert.Single(violations); - Assert.Contains("frontmatter", violations[0]); - } - - [Fact] - public void Validate_NameDoesNotMatchDirectory_ReportsMismatch() - { - var fm = new SkillFrontmatter("my-skill", "A description.", null, null, null, null); - var violations = SkillFrontmatterSpec.Validate(fm, "different-dir"); - Assert.Contains(violations, v => v.Contains("does not match")); - } - - [Fact] - public void Validate_FullyCompliant_ReturnsNoViolations() - { - var fm = new SkillFrontmatter("my-skill", "A description.", "MIT", "Requires docker", null, null); - var violations = SkillFrontmatterSpec.Validate(fm, "my-skill"); - Assert.Empty(violations); - } - - // ── ToSlug ─────────────────────────────────────────────────────────────── - - [Theory] - [InlineData("PDF Processing", "pdf-processing")] - [InlineData("My Bad Skill!!", "my-bad-skill")] - [InlineData(" leading and trailing ", "leading-and-trailing")] - [InlineData("already-a-slug", "already-a-slug")] - public void ToSlug_ProducesValidSlug(string input, string expected) - { - var slug = SkillFrontmatterSpec.ToSlug(input); - Assert.Equal(expected, slug); - Assert.True(SkillFrontmatterSpec.ValidateName(slug, out _)); - } - - // ── WithCanonicalName ──────────────────────────────────────────────────── - - [Fact] - public void WithCanonicalName_ReplacesExistingNameField() - { - var content = "---\nname: My Bad Skill!!\ndescription: A description.\n---\n\nBody"; - var rewritten = SkillFrontmatterSpec.WithCanonicalName(content, "my-bad-skill"); - - var fm = SkillFrontmatterSpec.TryParse(rewritten); - Assert.Equal("my-bad-skill", fm!.Name); - Assert.Equal("A description.", fm.Description); // untouched - Assert.Contains("Body", rewritten); // body untouched - } - - [Fact] - public void WithCanonicalName_NoExistingNameField_InsertsOne() - { - var content = "---\ndescription: A description.\n---\n\nBody"; - var rewritten = SkillFrontmatterSpec.WithCanonicalName(content, "new-slug"); - - var fm = SkillFrontmatterSpec.TryParse(rewritten); - Assert.Equal("new-slug", fm!.Name); - Assert.Equal("A description.", fm.Description); - } - - [Fact] - public void WithCanonicalName_NoFrontmatter_ReturnsContentUnchanged() - { - const string content = "# No frontmatter\n\nJust body text."; - Assert.Equal(content, SkillFrontmatterSpec.WithCanonicalName(content, "some-slug")); - } -} diff --git a/tests/FuseraftCli.Tests/SkillPathGuardTests.cs b/tests/FuseraftCli.Tests/SkillPathGuardTests.cs deleted file mode 100644 index 5669101c..00000000 --- a/tests/FuseraftCli.Tests/SkillPathGuardTests.cs +++ /dev/null @@ -1,155 +0,0 @@ -using fuseraft.Core.Skills; - -namespace FuseraftCli.Tests; - -/// -/// Tests for — path-containment and symlink-escape checks used by -/// SkillsPlugin (read_skill_resource/run_skill_script) before touching a -/// path the model supplied. -/// -public sealed class SkillPathGuardTests : IDisposable -{ - private readonly string _root; - private readonly string _skillDir; - private readonly string _outsideDir; - - public SkillPathGuardTests() - { - _root = Path.Combine(Path.GetTempPath(), "fuseraft_pathguard_tests_" + Guid.NewGuid().ToString("N")[..8]); - _skillDir = Path.Combine(_root, "my-skill"); - _outsideDir = Path.Combine(_root, "outside"); - Directory.CreateDirectory(_skillDir); - Directory.CreateDirectory(_outsideDir); - } - - public void Dispose() => Directory.Delete(_root, recursive: true); - - /// True on platforms/environments where creating symlinks is permitted (skips on restricted CI/sandboxes). - private bool CanCreateSymlinks(out string reason) - { - try - { - var link = Path.Combine(_root, "probe-link-" + Guid.NewGuid().ToString("N")[..6]); - File.CreateSymbolicLink(link, Path.Combine(_root, "probe-target")); - File.Delete(link); - reason = ""; - return true; - } - catch (Exception ex) - { - reason = ex.Message; - return false; - } - } - - [Fact] - public void TryResolveSafePath_PlainNestedFile_Succeeds() - { - Directory.CreateDirectory(Path.Combine(_skillDir, "references")); - File.WriteAllText(Path.Combine(_skillDir, "references", "guide.md"), "content"); - - var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "references/guide.md", out var fullPath, out var reason); - - Assert.True(ok); - Assert.Null(reason); - Assert.EndsWith(Path.Combine("references", "guide.md"), fullPath); - } - - [Theory] - [InlineData("../secret.txt")] - [InlineData("references/../../secret.txt")] - public void TryResolveSafePath_TraversalOutsideRoot_Fails(string relative) - { - File.WriteAllText(Path.Combine(_root, "secret.txt"), "top secret"); - - var ok = SkillPathGuard.TryResolveSafePath(_skillDir, relative, out _, out var reason); - - Assert.False(ok); - Assert.Contains("outside", reason); - } - - [Fact] - public void TryResolveSafePath_AbsolutePathEscape_Fails() - { - var outsideFile = Path.Combine(_outsideDir, "secret.txt"); - File.WriteAllText(outsideFile, "top secret"); - - var ok = SkillPathGuard.TryResolveSafePath(_skillDir, outsideFile, out _, out var reason); - - Assert.False(ok); - Assert.Contains("outside", reason); - } - - [Fact] - public void TryResolveSafePath_SymlinkedFilePointingOutside_Fails() - { - if (!CanCreateSymlinks(out _)) return; // environment doesn't allow symlinks — skip - - var secret = Path.Combine(_outsideDir, "secret.txt"); - File.WriteAllText(secret, "top secret"); - var link = Path.Combine(_skillDir, "innocuous.md"); - File.CreateSymbolicLink(link, secret); - - var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "innocuous.md", out _, out var reason); - - Assert.False(ok); - Assert.Contains("symlink", reason); - } - - [Fact] - public void TryResolveSafePath_SymlinkedSubdirectoryPointingOutside_Fails() - { - if (!CanCreateSymlinks(out _)) return; - - var secretDir = Path.Combine(_outsideDir, "secret-dir"); - Directory.CreateDirectory(secretDir); - File.WriteAllText(Path.Combine(secretDir, "file.txt"), "top secret"); - - var linkedDir = Path.Combine(_skillDir, "references"); - Directory.CreateSymbolicLink(linkedDir, secretDir); - - var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "references/file.txt", out _, out var reason); - - Assert.False(ok); - Assert.Contains("symlink", reason); - } - - [Fact] - public void TryResolveSafePath_NonExistentPath_StillReportsContainment() - { - // A not-yet-existing path inside the root should pass the guard; the caller's own - // File.Exists check is responsible for reporting "not found". - var ok = SkillPathGuard.TryResolveSafePath(_skillDir, "references/missing.md", out _, out var reason); - - Assert.True(ok); - Assert.Null(reason); - } - - [Fact] - public void IsReparsePoint_RegularFile_ReturnsFalse() - { - var file = Path.Combine(_skillDir, "plain.txt"); - File.WriteAllText(file, "content"); - - Assert.False(SkillPathGuard.IsReparsePoint(file)); - } - - [Fact] - public void IsReparsePoint_NonExistentPath_ReturnsFalse() - { - Assert.False(SkillPathGuard.IsReparsePoint(Path.Combine(_skillDir, "missing"))); - } - - [Fact] - public void IsReparsePoint_Symlink_ReturnsTrue() - { - if (!CanCreateSymlinks(out _)) return; - - var target = Path.Combine(_outsideDir, "target.txt"); - File.WriteAllText(target, "content"); - var link = Path.Combine(_skillDir, "link.txt"); - File.CreateSymbolicLink(link, target); - - Assert.True(SkillPathGuard.IsReparsePoint(link)); - } -} diff --git a/tests/FuseraftCli.Tests/SkillsHelpersTests.cs b/tests/FuseraftCli.Tests/SkillsHelpersTests.cs index 540187ba..61708576 100644 --- a/tests/FuseraftCli.Tests/SkillsHelpersTests.cs +++ b/tests/FuseraftCli.Tests/SkillsHelpersTests.cs @@ -1,4 +1,5 @@ using fuseraft.Cli.Commands.Skills; +using fuseraft.Core.Skills; namespace FuseraftCli.Tests; @@ -124,7 +125,7 @@ public void CanonicalizeName_NameDiffersFromSlug_RewritesNameField() var rewritten = SkillsHelpers.CanonicalizeName(content, "my-bad-skill"); Assert.Equal("my-bad-skill", SkillsHelpers.ExtractSlug(rewritten)); - Assert.Equal("A skill.", SkillsHelpers.ExtractDescription(rewritten)); + Assert.Equal("A skill.", FrontmatterFieldReader.ExtractField(rewritten, "description")); Assert.Contains("Body", rewritten); } } diff --git a/tests/FuseraftCli.Tests/SkillsPluginTests.cs b/tests/FuseraftCli.Tests/SkillsPluginTests.cs deleted file mode 100644 index 5a72aef0..00000000 --- a/tests/FuseraftCli.Tests/SkillsPluginTests.cs +++ /dev/null @@ -1,315 +0,0 @@ -using fuseraft.Infrastructure.Plugins; - -namespace FuseraftCli.Tests; - -/// -/// Tests for . -/// -/// Each test gets an isolated temp directory. All slug-to-dir entries in the plugin -/// point into that directory so no real skill library is touched. -/// -public sealed class SkillsPluginTests : IDisposable -{ - private readonly string _root; - - public SkillsPluginTests() - { - _root = Path.Combine(Path.GetTempPath(), "fuseraft_skills_tests_" + Guid.NewGuid().ToString("N")[..8]); - Directory.CreateDirectory(_root); - } - - public void Dispose() => Directory.Delete(_root, recursive: true); - - // ── helpers ────────────────────────────────────────────────────────────── - - private string MakeSkillDir(string slug, string? content = null) - { - var dir = Path.Combine(_root, slug); - Directory.CreateDirectory(dir); - if (content is not null) - File.WriteAllText(Path.Combine(dir, "SKILL.md"), content); - return dir; - } - - private SkillsPlugin PluginFor(params (string Slug, string? Content)[] skills) - { - var dirs = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var (slug, content) in skills) - dirs[slug] = MakeSkillDir(slug, content); - return new SkillsPlugin(dirs); - } - - // ── LoadSkillAsync ──────────────────────────────────────────────────────── - - [Fact] - public async Task LoadSkill_UnknownSlug_ReturnsNotFound() - { - var plugin = PluginFor(("my-skill", "content")); - var result = await plugin.LoadSkillAsync("does-not-exist"); - Assert.StartsWith("[NOT FOUND]", result); - Assert.Contains("does-not-exist", result); - } - - [Fact] - public async Task LoadSkill_UnknownSlug_ListsKnownSkillsInMessage() - { - var plugin = PluginFor(("alpha", "body"), ("beta", "body")); - var result = await plugin.LoadSkillAsync("gamma"); - Assert.Contains("alpha", result); - Assert.Contains("beta", result); - } - - [Fact] - public async Task LoadSkill_ValidSlug_ReturnsFileContent() - { - const string body = "## Do the thing\n1. Step one\n2. Step two"; - var plugin = PluginFor(("my-skill", body)); - var result = await plugin.LoadSkillAsync("my-skill"); - Assert.Equal(body, result); - } - - [Fact] - public async Task LoadSkill_EmptySkillFile_ReturnsEmptyString() - { - var plugin = PluginFor(("empty-skill", "")); - var result = await plugin.LoadSkillAsync("empty-skill"); - Assert.Equal(string.Empty, result); - } - - [Fact] - public async Task LoadSkill_SkillMdDeletedAfterInit_ReturnsError() - { - // TOCTOU: file disappears between plugin construction and the load call. - var dir = MakeSkillDir("vanishing", "some content"); - File.Delete(Path.Combine(dir, "SKILL.md")); - - var plugin = new SkillsPlugin(new Dictionary { ["vanishing"] = dir }); - var result = await plugin.LoadSkillAsync("vanishing"); - - Assert.StartsWith("[ERROR]", result); - } - - [Fact] - public async Task LoadSkill_SlugIsCaseInsensitive() - { - var plugin = PluginFor(("My-Skill", "body")); - var result = await plugin.LoadSkillAsync("my-skill"); - Assert.Equal("body", result); - } - - [Fact] - public async Task LoadSkill_DoesNotThrow_ReturnsStringResult() - { - // Any slug → result must be a string, never an unhandled exception. - var plugin = new SkillsPlugin(new Dictionary()); - var ex = await Record.ExceptionAsync(() => plugin.LoadSkillAsync("anything")); - Assert.Null(ex); - } - - // ── ReadSkillResourceAsync ──────────────────────────────────────────────── - - [Fact] - public async Task ReadSkillResource_UnknownSkill_ReturnsNotFound() - { - var plugin = PluginFor(("real-skill", "body")); - var result = await plugin.ReadSkillResourceAsync("ghost", "references/x.md"); - Assert.StartsWith("[NOT FOUND]", result); - } - - [Fact] - public async Task ReadSkillResource_EmptyPath_ReturnsError() - { - var plugin = PluginFor(("my-skill", "body")); - var result = await plugin.ReadSkillResourceAsync("my-skill", ""); - Assert.StartsWith("[ERROR]", result); - } - - [Fact] - public async Task ReadSkillResource_MissingFile_ReturnsNotFound() - { - var plugin = PluginFor(("my-skill", "body")); - var result = await plugin.ReadSkillResourceAsync("my-skill", "references/missing.md"); - Assert.StartsWith("[NOT FOUND]", result); - Assert.Contains("references/missing.md", result); - } - - [Fact] - public async Task ReadSkillResource_NestedFile_ReturnsContent() - { - var dir = MakeSkillDir("my-skill", "body"); - Directory.CreateDirectory(Path.Combine(dir, "references")); - File.WriteAllText(Path.Combine(dir, "references", "style-guide.md"), "# Style Guide\nUse tabs."); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.ReadSkillResourceAsync("my-skill", "references/style-guide.md"); - Assert.Equal("# Style Guide\nUse tabs.", result); - } - - [Theory] - [InlineData("../secret.txt")] - [InlineData("references/../../secret.txt")] - public async Task ReadSkillResource_PathTraversal_ReturnsError(string traversalPath) - { - var dir = MakeSkillDir("my-skill", "body"); - File.WriteAllText(Path.Combine(_root, "secret.txt"), "top secret"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.ReadSkillResourceAsync("my-skill", traversalPath); - Assert.StartsWith("[ERROR]", result); - Assert.DoesNotContain("top secret", result); - } - - [Fact] - public async Task ReadSkillResource_AbsolutePathEscape_ReturnsError() - { - var dir = MakeSkillDir("my-skill", "body"); - var outsideFile = Path.Combine(_root, "secret.txt"); - File.WriteAllText(outsideFile, "top secret"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.ReadSkillResourceAsync("my-skill", outsideFile); - Assert.StartsWith("[ERROR]", result); - Assert.DoesNotContain("top secret", result); - } - - [Fact] - public async Task ReadSkillResource_SymlinkedFilePointingOutside_ReturnsError() - { - var dir = MakeSkillDir("my-skill", "body"); - var secret = Path.Combine(_root, "secret.txt"); - File.WriteAllText(secret, "top secret"); - - string link; - try - { - link = Path.Combine(dir, "innocuous.md"); - File.CreateSymbolicLink(link, secret); - } - catch (Exception) - { - return; // environment doesn't allow symlinks — skip - } - - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - var result = await plugin.ReadSkillResourceAsync("my-skill", "innocuous.md"); - - Assert.StartsWith("[ERROR]", result); - Assert.DoesNotContain("top secret", result); - } - - // ── RunSkillScriptAsync ─────────────────────────────────────────────────── - - [Fact] - public async Task RunSkillScript_UnknownSkill_ReturnsNotFound() - { - var plugin = PluginFor(("real-skill", "body")); - var result = await plugin.RunSkillScriptAsync("ghost", "run.sh"); - Assert.StartsWith("[NOT FOUND]", result); - } - - [Fact] - public async Task RunSkillScript_PathTraversal_ReturnsError() - { - var dir = MakeSkillDir("my-skill", "body"); - var outsideScript = Path.Combine(_root, "evil.sh"); - File.WriteAllText(outsideScript, "#!/bin/sh\necho pwned\n"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.RunSkillScriptAsync("my-skill", "../evil.sh"); - Assert.StartsWith("[ERROR]", result); - Assert.DoesNotContain("pwned", result); - } - - [Fact] - public async Task RunSkillScript_SymlinkedScriptPointingOutside_ReturnsError() - { - var dir = MakeSkillDir("my-skill", "body"); - var outsideScript = Path.Combine(_root, "evil.sh"); - File.WriteAllText(outsideScript, "#!/bin/sh\necho pwned\n"); - - string link; - try - { - link = Path.Combine(dir, "run.sh"); - File.CreateSymbolicLink(link, outsideScript); - } - catch (Exception) - { - return; // environment doesn't allow symlinks — skip - } - - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - var result = await plugin.RunSkillScriptAsync("my-skill", "run.sh"); - - Assert.StartsWith("[ERROR]", result); - Assert.DoesNotContain("pwned", result); - } - - [Fact] - public async Task RunSkillScript_NestedScriptPath_Runs() - { - var dir = MakeSkillDir("my-skill", "body"); - Directory.CreateDirectory(Path.Combine(dir, "scripts")); - File.WriteAllText(Path.Combine(dir, "scripts", "hello.sh"), "#!/bin/sh\necho nested-ok\n"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.RunSkillScriptAsync("my-skill", "scripts/hello.sh"); - Assert.Contains("nested-ok", result); - } - - [Fact] - public async Task RunSkillScript_ScriptFileMissing_ReturnsNotFound() - { - var plugin = PluginFor(("my-skill", "body")); - var result = await plugin.RunSkillScriptAsync("my-skill", "missing.sh"); - Assert.StartsWith("[NOT FOUND]", result); - Assert.Contains("missing.sh", result); - } - - [Fact] - public async Task RunSkillScript_UnsupportedExtension_ReturnsError() - { - var dir = MakeSkillDir("my-skill", "body"); - File.WriteAllText(Path.Combine(dir, "run.exe"), "binary"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.RunSkillScriptAsync("my-skill", "run.exe"); - Assert.StartsWith("[ERROR]", result); - Assert.Contains(".exe", result); - } - - [Fact] - public async Task RunSkillScript_ShellScript_ReturnsStdout() - { - var dir = MakeSkillDir("my-skill", "body"); - File.WriteAllText(Path.Combine(dir, "hello.sh"), "#!/bin/sh\necho hello-from-skill\n"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.RunSkillScriptAsync("my-skill", "hello.sh"); - Assert.Contains("hello-from-skill", result); - } - - [Fact] - public async Task RunSkillScript_ScriptWritesToStderr_StderrAppendedToResult() - { - var dir = MakeSkillDir("my-skill", "body"); - File.WriteAllText(Path.Combine(dir, "warn.sh"), "#!/bin/sh\necho out\necho err >&2\n"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var result = await plugin.RunSkillScriptAsync("my-skill", "warn.sh"); - Assert.Contains("out", result); - Assert.Contains("stderr", result); - Assert.Contains("err", result); - } - - [Fact] - public async Task RunSkillScript_EmptyArgs_DoesNotThrow() - { - var dir = MakeSkillDir("my-skill", "body"); - File.WriteAllText(Path.Combine(dir, "noop.sh"), "#!/bin/sh\necho ok\n"); - var plugin = new SkillsPlugin(new Dictionary { ["my-skill"] = dir }); - - var ex = await Record.ExceptionAsync(() => plugin.RunSkillScriptAsync("my-skill", "noop.sh", args: "")); - Assert.Null(ex); - } -}