diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs index 2a43814b604..123c4100b40 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs @@ -2,7 +2,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Text.Json; using System.Threading; @@ -22,19 +21,22 @@ namespace Microsoft.Agents.AI; /// /// /// Discovery follows the SEP-2640 recommended approach: the source reads the well-known -/// skill://index.json resource and constructs one per -/// skill-md entry directly from the entry's name, description, and url fields. -/// The referenced SKILL.md resource is not read during discovery; hosts fetch its body on -/// demand via resources/read against the URI exposed on the resulting skill. +/// skill://index.json resource and constructs one per index entry. /// /// -/// Only index entries of type skill-md are supported at the moment; entries of any other -/// type are skipped. +/// Index entries are dispatched to an by their type: +/// +/// skill-md - handled by ; the skill's +/// SKILL.md and sibling resources are fetched on demand from the MCP server. +/// archive - handled by ; the entry's +/// url points to a single archive resource whose content unpacks into the skill's +/// namespace. +/// +/// Entries whose type has no registered loader (e.g. mcp-resource-template) are skipped. /// /// -/// If skill://index.json is absent, unreadable, empty, or fails to parse, this source -/// returns an empty list. Discovered skills serve their referenced resources on demand via -/// ; they do not enumerate sibling files up front. +/// If skill://index.json is absent, unreadable, empty, or fails to parse, this source returns an +/// empty list. /// /// internal sealed partial class AgentMcpSkillsSource : AgentSkillsSource @@ -44,42 +46,148 @@ internal sealed partial class AgentMcpSkillsSource : AgentSkillsSource /// private const string IndexUri = "skill://index.json"; - private const string SkillMdEntryType = "skill-md"; - private readonly McpClient _client; private readonly ILogger _logger; + private readonly Dictionary _loaders; + private readonly TimeSpan? _refreshInterval; + + private IList? _cachedSkills; + private DateTime _lastRefreshedUtc; + private Task>? _refreshTask; /// /// Initializes a new instance of the class. /// /// An MCP client connected to a server that exposes Agent Skills resources. + /// Optional options that control archive-distributed skill handling. /// Optional logger factory. - public AgentMcpSkillsSource(McpClient client, ILoggerFactory? loggerFactory = null) + public AgentMcpSkillsSource(McpClient client, AgentMcpSkillsSourceOptions? options = null, ILoggerFactory? loggerFactory = null) { this._client = Throw.IfNull(client); - this._logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger(); + loggerFactory ??= NullLoggerFactory.Instance; + this._logger = loggerFactory.CreateLogger(); + + IMcpSkillEntryLoader[] loaders = + [ + new SkillMdEntryLoader(this._client, loggerFactory), + new ArchiveEntryLoader(this._client, options, loggerFactory), + ]; + + this._loaders = loaders.ToDictionary(l => l.EntryType, StringComparer.OrdinalIgnoreCase); + this._refreshInterval = options?.RefreshInterval; } /// public override async Task> GetSkillsAsync(CancellationToken cancellationToken = default) + { + if (this.TryGetCachedSkills() is { } cached) + { + return cached; + } + + // Use CAS to ensure only one concurrent refresh runs; other callers await the same task. + var tcs = new TaskCompletionSource>(TaskCreationOptions.RunContinuationsAsynchronously); + + if (Interlocked.CompareExchange(ref this._refreshTask, tcs.Task, null) is { } existing) + { + // Wait for the in-flight refresh but let this caller cancel its own wait independently + // without aborting the shared refresh work. + return await existing.WaitAsync(cancellationToken).ConfigureAwait(false); + } + + try + { + // The refresh owner uses CancellationToken.None so that a single caller's cancellation + // does not abort the shared refresh for all concurrent waiters. + var skills = await this.GetCoreSkillsAsync(CancellationToken.None).ConfigureAwait(false); + + this.UpdateCache(skills); + + tcs.SetResult(skills); + + // Allow the current caller to observe cancellation without impacting other awaiters. + cancellationToken.ThrowIfCancellationRequested(); + + return skills; + } + catch (Exception ex) + { + tcs.TrySetException(ex); + throw; + } + finally + { + this._refreshTask = null; + } + } + + /// + /// Returns the cached skill list if caching is enabled and the cache is still fresh; + /// otherwise returns . + /// + private IList? TryGetCachedSkills() + { + if (this._refreshInterval is null || this._cachedSkills is null) + { + return null; + } + + TimeSpan cacheAge = DateTime.UtcNow - this._lastRefreshedUtc; + + if (cacheAge >= this._refreshInterval.Value) + { + return null; + } + + return this._cachedSkills; + } + + /// + /// Stores the skill list and records the refresh timestamp for cache freshness checks. + /// + private void UpdateCache(IList skills) + { + this._cachedSkills = skills; + this._lastRefreshedUtc = DateTime.UtcNow; + } + + /// + /// Reads the skill index from the MCP server, dispatches entries to registered loaders, and + /// returns the aggregated skill list. + /// + private async Task> GetCoreSkillsAsync(CancellationToken cancellationToken) { McpSkillIndex? index = await this.TryReadIndexAsync(cancellationToken).ConfigureAwait(false); - var skills = new List(); + // Group entries by type and set aside those a registered loader can handle; entries of any + // other type are unsupported and logged. + var entriesByType = new Dictionary>(StringComparer.OrdinalIgnoreCase); - foreach (var entry in index?.Skills ?? []) + foreach (var group in (index?.Skills ?? []).GroupBy(e => e.Type ?? string.Empty, StringComparer.OrdinalIgnoreCase)) { - if (this.TryCreateSkill(entry, out AgentMcpSkill? skill, out string skipReason)) + if (this._loaders.ContainsKey(group.Key)) { - skills.Add(skill); - LogSkillLoaded(this._logger, skill.Frontmatter.Name); + entriesByType[group.Key] = group.ToList(); } else { - LogIndexEntrySkipped(this._logger, entry.Name ?? "(unnamed)", skipReason); + foreach (var entry in group) + { + LogIndexEntrySkipped(this._logger, entry.Name ?? "(unnamed)", $"unsupported type '{entry.Type ?? "(none)"}'"); + } } } + // Invoke every registered loader, even when the server advertises no entries of its type, so + // each type's lifecycle still runs (e.g. the archive loader prunes leftover directories). + var skills = new List(); + + foreach (var loader in this._loaders.Values) + { + var entries = entriesByType.TryGetValue(loader.EntryType, out List? matched) ? matched : []; + skills.AddRange(await loader.LoadAsync(entries, cancellationToken).ConfigureAwait(false)); + } + LogSkillsLoadedTotal(this._logger, skills.Count); return skills; @@ -124,44 +232,6 @@ public override async Task> GetSkillsAsync(CancellationToken c } } - private bool TryCreateSkill( - McpSkillIndexEntry entry, - [NotNullWhen(true)] out AgentMcpSkill? skill, - out string skipReason) - { - skill = null; - - if (!string.Equals(entry.Type, SkillMdEntryType, StringComparison.Ordinal)) - { - skipReason = $"unsupported type '{entry.Type ?? "(none)"}'"; - return false; - } - - if (string.IsNullOrWhiteSpace(entry.Url)) - { - skipReason = "missing required 'url' field"; - return false; - } - - AgentSkillFrontmatter frontmatter; - try - { - frontmatter = new AgentSkillFrontmatter(entry.Name!, entry.Description!); - } - catch (ArgumentException ex) - { - skipReason = $"invalid metadata: {ex.Message}"; - return false; - } - - skill = new AgentMcpSkill(frontmatter, entry.Url!, this._client); - skipReason = string.Empty; - return true; - } - - [LoggerMessage(LogLevel.Information, "Loaded MCP skill: {SkillName}")] - private static partial void LogSkillLoaded(ILogger logger, string skillName); - [LoggerMessage(LogLevel.Information, "Successfully loaded {Count} skills from MCP server")] private static partial void LogSkillsLoadedTotal(ILogger logger, int count); diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSourceOptions.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSourceOptions.cs new file mode 100644 index 00000000000..78cf93881e1 --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSourceOptions.cs @@ -0,0 +1,98 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using Microsoft.Shared.DiagnosticIds; + +namespace Microsoft.Agents.AI; + +/// +/// Configuration options for . +/// +[Experimental(DiagnosticIds.Experiments.AgentsAIExperiments)] +public sealed class AgentMcpSkillsSourceOptions +{ + /// + /// Gets or sets the base directory that archive-type skills are extracted to and served from. + /// + /// + /// Archives are extracted beneath this directory as {ArchiveSkillsDirectory}/{skill-name}/. + /// When , the source extracts to a per-instance unique location of + /// {currentDirectory}/{guid}/{skill-name}/, where the GUID is generated once per + /// instance so that multiple sources never overwrite one + /// another. Set this to a fixed value to get a predictable, reusable extraction location. + /// When set, each source must use its own unique directory: the source treats the directory as + /// exclusively its own and, on every discovery, prunes any sub-directory that the MCP server no + /// longer advertises or whose index entry is not actionable (e.g., missing a required field). + /// Pointing two sources at the same directory would therefore cause them to + /// delete each other's extracted skills. + /// + public string? ArchiveSkillsDirectory { get; set; } + + /// + /// Gets or sets the allowed file extensions for resources discovered in extracted archive-type skills. + /// + /// + /// When , defaults to .md, .json, .yaml, .yml, + /// .csv, .xml, and .txt. + /// + public IEnumerable? ArchiveResourceExtensions { get; set; } + + /// + /// Gets or sets the maximum depth to search for resource files within each extracted archive-type + /// skill directory. A value of 1 searches only the skill root directory. A value of 2 + /// searches the root and one level of subdirectories. + /// + /// + /// When , the source uses the default depth of 2. + /// + public int? ArchiveResourceSearchDepth { get; set; } + + /// + /// Gets or sets the maximum number of files that may be extracted from a single archive-type skill. + /// + /// + /// Guards against excessive-file-count denial-of-service archives. When , the + /// source uses a default of 20, sized for a typical well-formed skill (a handful of files). + /// Raise this for archive-type skills that legitimately bundle many files. An archive that exceeds + /// the limit is skipped. + /// + public int? ArchiveMaxFileCount { get; set; } + + /// + /// Gets or sets the maximum size, in bytes, of a downloaded archive-type skill resource. + /// + /// + /// Guards against archive resources that are too large to materialize safely. When + /// , the source uses a default of 1 MB, sized for a typical + /// well-formed skill archive. Raise this for archive-type skills that legitimately require + /// larger archive payloads. An archive that exceeds the limit is skipped. + /// + public long? ArchiveMaxSizeBytes { get; set; } + + /// + /// Gets or sets the maximum total uncompressed size, in bytes, of all files extracted from a single + /// archive-type skill. + /// + /// + /// Guards against decompression-bomb archives. When , the source uses a default + /// of 1 MB, sized for a typical well-formed skill (well under ~1 MB). Raise this for + /// archive-type skills that legitimately bundle larger content. An archive that exceeds the limit is + /// skipped. + /// + public long? ArchiveMaxUncompressedSizeBytes { get; set; } + + /// + /// Gets or sets the interval at which cached skills are considered fresh. When a caller invokes + /// and the cached result is younger than this + /// interval, the cached list is returned without contacting the MCP server. + /// + /// + /// When (the default), caching is disabled and every call fetches from + /// the MCP server. Set to a positive to enable caching. Values of + /// or negative durations effectively disable caching because the + /// cache age will always be greater than or equal to the interval. + /// + public TimeSpan? RefreshInterval { get; set; } +} diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentSkillsProviderBuilderMcpExtensions.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentSkillsProviderBuilderMcpExtensions.cs index 9b90f36511f..a7af8fdcec3 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentSkillsProviderBuilderMcpExtensions.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentSkillsProviderBuilderMcpExtensions.cs @@ -15,12 +15,13 @@ public static class AgentSkillsProviderBuilderMcpExtensions /// /// The builder to extend. /// An MCP client connected to a server exposing Agent Skills resources. + /// Optional options that control archive-distributed skill handling. /// The builder instance for chaining. - public static AgentSkillsProviderBuilder UseMcpSkills(this AgentSkillsProviderBuilder builder, McpClient client) + public static AgentSkillsProviderBuilder UseMcpSkills(this AgentSkillsProviderBuilder builder, McpClient client, AgentMcpSkillsSourceOptions? options = null) { _ = Throw.IfNull(builder); _ = Throw.IfNull(client); - return builder.UseSource(new AgentMcpSkillsSource(client)); + return builder.UseSource(loggerFactory => new AgentMcpSkillsSource(client, options, loggerFactory)); } } diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs new file mode 100644 index 00000000000..15e37926a77 --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs @@ -0,0 +1,286 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Buffers; +using System.Formats.Tar; +using System.IO; +using System.IO.Compression; + +namespace Microsoft.Agents.AI; + +/// +/// Unpacks skill archives downloaded from an MCP server into a local directory. +/// +/// +/// Supports ZIP, TAR, and gzip-compressed TAR payloads. Extraction is guarded against path-traversal +/// ("zip-slip") attacks: every entry must resolve to a path beneath the target directory. Non-regular +/// TAR entries (symbolic links, hard links, device nodes, etc.) are skipped so an archive cannot +/// create links that escape the target directory. Extraction is also bounded by a maximum file count +/// and total uncompressed size to mitigate decompression-bomb attacks. +/// +internal static class AgentMcpSkillArchiveExtractor +{ + /// + /// The default maximum number of files that may be extracted from a single archive, sized for a + /// typical well-formed skill (a handful of files) with headroom over the observed average. + /// + internal const int DefaultMaxFileCount = 20; + + /// + /// The default maximum total uncompressed size, in bytes, of all files extracted from a single + /// archive, sized for a typical well-formed skill (well under ~1 MB). + /// + internal const long DefaultMaxUncompressedSizeBytes = 1L * 1024 * 1024; + + private const int CopyBufferSize = 81920; + + /// + /// Determines the archive format from the advertised media type, the source URL, and the leading + /// bytes of the payload (magic-number sniffing takes precedence as it is the most reliable). + /// + /// The archive bytes. + /// The advertised MIME type, if any. + /// The resource URL the archive was read from, if any. + /// The detected , or . + internal static ArchiveFormat DetectFormat(byte[] bytes, string? mediaType, string? url) + { + // Magic-number sniffing is the most reliable signal. + if (bytes.Length >= 2 && bytes[0] == 0x1F && bytes[1] == 0x8B) + { + return ArchiveFormat.TarGz; + } + + if (bytes.Length >= 4 && bytes[0] == 0x50 && bytes[1] == 0x4B && + (bytes[2] == 0x03 || bytes[2] == 0x05 || bytes[2] == 0x07)) + { + return ArchiveFormat.Zip; + } + + string media = mediaType?.Trim() ?? string.Empty; + if (string.Equals(media, "application/zip", StringComparison.OrdinalIgnoreCase) || + string.Equals(media, "application/x-zip-compressed", StringComparison.OrdinalIgnoreCase)) + { + return ArchiveFormat.Zip; + } + + if (string.Equals(media, "application/gzip", StringComparison.OrdinalIgnoreCase) || + string.Equals(media, "application/x-gzip", StringComparison.OrdinalIgnoreCase) || + string.Equals(media, "application/x-compressed-tar", StringComparison.OrdinalIgnoreCase)) + { + return ArchiveFormat.TarGz; + } + + if (string.Equals(media, "application/x-tar", StringComparison.OrdinalIgnoreCase) || + string.Equals(media, "application/tar", StringComparison.OrdinalIgnoreCase)) + { + return ArchiveFormat.Tar; + } + + string u = url ?? string.Empty; + if (u.EndsWith(".zip", StringComparison.OrdinalIgnoreCase)) + { + return ArchiveFormat.Zip; + } + + if (u.EndsWith(".tar.gz", StringComparison.OrdinalIgnoreCase) || u.EndsWith(".tgz", StringComparison.OrdinalIgnoreCase)) + { + return ArchiveFormat.TarGz; + } + + if (u.EndsWith(".tar", StringComparison.OrdinalIgnoreCase)) + { + return ArchiveFormat.Tar; + } + + return ArchiveFormat.Unknown; + } + + /// + /// Extracts the supplied archive into . + /// + /// The archive bytes. + /// The archive container format. + /// The directory the archive is unpacked into. Created if missing. + /// The maximum number of files that may be extracted from the archive. + /// The maximum total uncompressed size, in bytes, of all extracted files. + /// The format is . + /// The archive exceeds one of the supplied limits. + internal static void Extract( + byte[] bytes, + ArchiveFormat format, + string targetDirectory, + int? maxFileCount = null, + long? maxUncompressedSizeBytes = null) + { + maxFileCount ??= DefaultMaxFileCount; + maxUncompressedSizeBytes ??= DefaultMaxUncompressedSizeBytes; + + Directory.CreateDirectory(targetDirectory); + string fullTarget = Path.GetFullPath(targetDirectory); + + using var source = new MemoryStream(bytes, writable: false); + + switch (format) + { + case ArchiveFormat.Zip: + ExtractZip(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); + break; + case ArchiveFormat.Tar: + ExtractTar(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); + break; + case ArchiveFormat.TarGz: + { + using var gzip = new GZipStream(source, CompressionMode.Decompress); + ExtractTar(gzip, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); + break; + } + default: + throw new NotSupportedException($"Unsupported skill archive format '{format}'."); + } + } + + private static void ExtractZip(Stream source, string fullTarget, int maxFileCount, long maxUncompressedSizeBytes) + { + using var archive = new ZipArchive(source, ZipArchiveMode.Read, leaveOpen: true); + + long remainingBytes = maxUncompressedSizeBytes; + int fileCount = 0; + + foreach (ZipArchiveEntry entry in archive.Entries) + { + // Directory entries have an empty Name. + if (entry.Name.Length == 0) + { + continue; + } + + if (++fileCount > maxFileCount) + { + throw new InvalidDataException($"Skill archive exceeds the maximum allowed file count ({maxFileCount})."); + } + + string? destination = ResolveDestination(fullTarget, entry.FullName); + if (destination is null) + { + continue; + } + + Directory.CreateDirectory(Path.GetDirectoryName(destination)!); + + using Stream entryStream = entry.Open(); + using FileStream output = File.Create(destination); + CopyWithLimit(entryStream, output, ref remainingBytes); + } + } + + private static void ExtractTar(Stream source, string fullTarget, int maxFileCount, long maxUncompressedSizeBytes) + { + using var reader = new TarReader(source, leaveOpen: true); + + long remainingBytes = maxUncompressedSizeBytes; + int fileCount = 0; + + while (reader.GetNextEntry() is { } entry) + { + // Only regular files are materialized. Skipping links/devices avoids both unsupported + // entry types and link-based escapes outside the target directory. + if (entry.EntryType is not (TarEntryType.RegularFile or TarEntryType.V7RegularFile)) + { + continue; + } + + if (++fileCount > maxFileCount) + { + throw new InvalidDataException($"Skill archive exceeds the maximum allowed file count ({maxFileCount})."); + } + + string? destination = ResolveDestination(fullTarget, entry.Name); + if (destination is null || entry.DataStream is null) + { + continue; + } + + Directory.CreateDirectory(Path.GetDirectoryName(destination)!); + using FileStream output = File.Create(destination); + CopyWithLimit(entry.DataStream, output, ref remainingBytes); + } + } + + /// + /// Copies to while decrementing a shared + /// uncompressed-byte budget, throwing once it is exhausted. This is the authoritative defense against + /// decompression bombs because it counts bytes actually produced by the decompressor rather than + /// trusting archive metadata which may be inaccurate. Peak memory is bounded to a single buffer. + /// + /// The remaining byte budget is exceeded. + private static void CopyWithLimit(Stream source, Stream destination, ref long remainingBytes) + { + byte[] buffer = ArrayPool.Shared.Rent(CopyBufferSize); + try + { + int read; + while ((read = source.Read(buffer, 0, buffer.Length)) > 0) + { + remainingBytes -= read; + if (remainingBytes < 0) + { + throw new InvalidDataException("Skill archive exceeds the maximum allowed uncompressed size."); + } + + destination.Write(buffer, 0, read); + } + } + finally + { + ArrayPool.Shared.Return(buffer); + } + } + + /// + /// Resolves an archive entry path to an absolute destination beneath , + /// or when the entry would escape the target directory (zip-slip). + /// + private static string? ResolveDestination(string fullTarget, string entryPath) + { + if (string.IsNullOrWhiteSpace(entryPath)) + { + return null; + } + + // Normalize separators and reject absolute / rooted entry paths outright. + string normalized = entryPath.Replace('\\', '/').TrimStart('/'); + if (normalized.Length == 0 || Path.IsPathRooted(normalized)) + { + return null; + } + + string destination = Path.GetFullPath(Path.Combine(fullTarget, normalized)); + if (!IsPathContainedIn(fullTarget, destination)) + { + return null; + } + + return destination; + } + + /// + /// Returns when resolves to a location + /// beneath . + /// + internal static bool IsPathContainedIn(string parentDirectory, string candidatePath) + { + string fullParent = Path.GetFullPath(parentDirectory); + string fullCandidate = Path.GetFullPath(candidatePath); + + // Append a trailing separator so the containment check doesn't false-match sibling + // directories. e.g. "/skills/myskill" matches "/skills/myskill-evil/", but + // "/skills/myskill/" does not. + string prefix = fullParent.EndsWith(Path.DirectorySeparatorChar.ToString(), StringComparison.Ordinal) + ? fullParent + : fullParent + Path.DirectorySeparatorChar; + + return fullCandidate.StartsWith( + prefix, + OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); + } +} diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs new file mode 100644 index 00000000000..b483eefa4f2 --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs @@ -0,0 +1,403 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using ModelContextProtocol; +using ModelContextProtocol.Client; +using ModelContextProtocol.Protocol; + +namespace Microsoft.Agents.AI; + +/// +/// Loads archive index entries: each entry's url points to a single archive resource +/// (application/zip, application/x-tar, or gzip-compressed TAR) whose content unpacks +/// into the skill's namespace. Archives are downloaded, extracted to a local directory, and the +/// resulting files are discovered via an internal that this +/// loader proxies to. +/// +/// +/// Because MCP-delivered skills are treated strictly as instructor-format text, scripts bundled +/// inside an archive are surfaced as readable resources only; they are never discovered as +/// executable scripts. +/// +internal sealed partial class ArchiveEntryLoader : IMcpSkillEntryLoader +{ + /// + /// The default maximum size, in bytes, of a downloaded archive resource. + /// + internal const long DefaultMaxArchiveSizeBytes = 1L * 1024 * 1024; + + private readonly McpClient _client; + private readonly AgentMcpSkillsSourceOptions? _options; + private readonly ILoggerFactory _loggerFactory; + private readonly ILogger _logger; + private string? _archiveSkillsDirectory; + + public ArchiveEntryLoader(McpClient client, AgentMcpSkillsSourceOptions? options, ILoggerFactory loggerFactory) + { + this._client = client; + this._options = options; + this._loggerFactory = loggerFactory; + this._logger = loggerFactory.CreateLogger(); + } + + /// + public string EntryType => "archive"; + + /// + public async Task> LoadAsync(IReadOnlyList entries, CancellationToken cancellationToken) + { + // Filter out entries that are missing required fields or have invalid names. + var archiveEntries = this.FilterValidEntries(entries); + + // Determine the target directory from prior state or caller-supplied options. + var archiveSkillsDirectory = this._archiveSkillsDirectory ?? this._options?.ArchiveSkillsDirectory; + + // Reconcile on-disk state with the current set of advertised skills. + this.ReconcileArchiveSkillDirectories(archiveSkillsDirectory, archiveEntries); + + if (archiveEntries.Count == 0) + { + return []; + } + + // Resolve or generate the skills directory and ensure it exists on disk. + this._archiveSkillsDirectory = archiveSkillsDirectory ?? Path.Combine(Directory.GetCurrentDirectory(), Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(this._archiveSkillsDirectory); + + // Download and extract each archive entry into its own subdirectory. + var skillDirectories = new List(archiveEntries.Count); + + foreach (var entry in archiveEntries) + { + skillDirectories.AddRange(await this.TryDownloadAndExtractSkillAsync(entry, this._archiveSkillsDirectory, cancellationToken).ConfigureAwait(false)); + } + + // Delegate discovery of extracted content to a file-based skills source. + AgentFileSkillsSource fileSource = this.CreateFileSkillsSource(skillDirectories); + + return await fileSource.GetSkillsAsync(cancellationToken).ConfigureAwait(false); + } + + /// + /// Reconciles the on-disk archive skill directories with the current set of advertised entries. + /// When no entries are advertised, all existing directories are removed. Otherwise, only stale + /// directories (not in the current advertised set) are pruned. + /// + private void ReconcileArchiveSkillDirectories(string? archiveSkillsDirectory, List archiveEntries) + { + // Nothing to reconcile if the directory hasn't been established yet. + if (string.IsNullOrEmpty(archiveSkillsDirectory) || !Directory.Exists(archiveSkillsDirectory)) + { + return; + } + + // Server advertises no archive skills - remove all previously extracted directories. + if (archiveEntries.Count == 0) + { + this.ClearSkillsDirectory(archiveSkillsDirectory); + return; + } + + // Compare the advertised set against what's on disk; prune any stale leftovers. + var advertisedSkillNames = archiveEntries + .Select(e => e.Name!) + .ToHashSet(OperatingSystem.IsWindows() ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); + + foreach (string directory in Directory.EnumerateDirectories(archiveSkillsDirectory)) + { + string name = Path.GetFileName(directory); + if (advertisedSkillNames.Contains(name)) + { + continue; + } + + try + { + Directory.Delete(directory, recursive: true); + LogArchiveSkillPruned(this._logger, name, SanitizePathForLog(directory)); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + LogArchiveSkillPruneFailed(this._logger, name, ex); + } + } + } + + /// + /// Removes all subdirectories of the specified directory. Failures on individual directories + /// are swallowed so that a single locked directory does not prevent cleanup of the others. + /// + private void ClearSkillsDirectory(string archiveSkillsDirectory) + { + foreach (var dir in Directory.GetDirectories(archiveSkillsDirectory)) + { + try + { + Directory.Delete(dir, true); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + LogArchiveSkillPruneFailed(this._logger, Path.GetFileName(dir), ex); + } + } + } + + /// + /// Creates the internal that discovers extracted archive + /// content. Scripts are never treated as executable; the allowed script extensions list is left + /// empty so that script files are not discovered as runnable scripts. + /// + private AgentFileSkillsSource CreateFileSkillsSource(IReadOnlyList archiveSkillDirectories) + { + var fileOptions = new AgentFileSkillsSourceOptions + { + AllowedScriptExtensions = [], + AllowedResourceExtensions = this._options?.ArchiveResourceExtensions, + SearchDepth = this._options?.ArchiveResourceSearchDepth, + }; + + return new AgentFileSkillsSource( + archiveSkillDirectories, + scriptRunner: null, + options: fileOptions, + loggerFactory: this._loggerFactory); + } + + /// + /// Downloads the archive for a single skill entry, detects its format, and extracts it into a + /// subdirectory of . Entries that cannot be materialized + /// (path escape, download failure, unsupported format, or extraction error) are skipped and logged; + /// partial content is cleaned up on failure. + /// + private async Task> TryDownloadAndExtractSkillAsync(McpSkillIndexEntry entry, string archiveSkillsDirectory, CancellationToken cancellationToken) + { + string skillDirectory = Path.Combine(archiveSkillsDirectory, entry.Name!); + + // Defense-in-depth: the skill directory is derived from the server-supplied entry name. + // Even though invalid names are filtered upstream, verify the resolved path cannot escape + // the archive skills directory before any file system operation is performed. + if (!AgentMcpSkillArchiveExtractor.IsPathContainedIn(archiveSkillsDirectory, skillDirectory)) + { + LogIndexEntrySkipped(this._logger, entry.Name!, "resolved skill directory escapes the archive skills directory"); + return []; + } + + // Clean the target directory so that content is always freshly extracted. + if (!this.TryDeleteSkillDirectory(entry.Name!, skillDirectory)) + { + return []; + } + + (byte[]? bytes, string? mimeType) = await this.DownloadSkillBytesAsync(entry, cancellationToken).ConfigureAwait(false); + if (bytes is null) + { + return []; + } + + var format = AgentMcpSkillArchiveExtractor.DetectFormat(bytes, mimeType, entry.Url); + if (format == ArchiveFormat.Unknown) + { + LogIndexEntrySkipped(this._logger, entry.Name!, $"unsupported archive media type '{mimeType ?? "(none)"}'"); + return []; + } + + try + { + AgentMcpSkillArchiveExtractor.Extract( + bytes, + format, + skillDirectory, + this._options?.ArchiveMaxFileCount, + this._options?.ArchiveMaxUncompressedSizeBytes); + } + catch (Exception ex) + { + LogArchiveExtractFailed(this._logger, entry.Name!, ex); + + // Remove any partially-extracted content so it is not later treated as a valid, + // already-materialized skill by the reuse check above. + _ = this.TryDeleteSkillDirectory(entry.Name!, skillDirectory); + return []; + } + + LogArchiveExtracted(this._logger, entry.Name!, SanitizePathForLog(skillDirectory)); + + return [skillDirectory]; + } + + /// + /// Downloads and decodes the binary content of a skill's archive resource. Returns + /// bytes when the resource cannot be read, contains no binary content, or is empty. + /// + private async Task<(byte[]? Bytes, string? MimeType)> DownloadSkillBytesAsync(McpSkillIndexEntry entry, CancellationToken cancellationToken) + { + ReadResourceResult result; + + try + { +#pragma warning disable CA2234 // Pass system uri objects instead of strings + result = await this._client.ReadResourceAsync(entry.Url!, cancellationToken: cancellationToken).ConfigureAwait(false); +#pragma warning restore CA2234 // Pass system uri objects instead of strings + } + catch (McpException ex) + { + LogArchiveReadFailed(this._logger, entry.Name!, ex); + return (null, null); + } + + BlobResourceContents? blobContent = result.Contents.OfType().FirstOrDefault(); + if (blobContent is null) + { + LogIndexEntrySkipped(this._logger, entry.Name!, "archive resource returned no binary content"); + return (null, null); + } + + long maxArchiveSizeBytes = this._options?.ArchiveMaxSizeBytes ?? DefaultMaxArchiveSizeBytes; + + byte[] bytes; + try + { + if (blobContent.DecodedData.Length > maxArchiveSizeBytes) + { + LogIndexEntrySkipped(this._logger, entry.Name!, $"archive resource exceeds the maximum allowed size ({maxArchiveSizeBytes} bytes)"); + return (null, null); + } + + bytes = blobContent.DecodedData.ToArray(); + } + catch (FormatException ex) + { + LogArchiveDecodeFailed(this._logger, entry.Name!, ex); + return (null, null); + } + + if (bytes.Length == 0) + { + LogIndexEntrySkipped(this._logger, entry.Name!, "archive resource returned empty content"); + return (null, null); + } + + return (bytes, blobContent.MimeType); + } + + /// + /// Filters archive entries to those that are valid for materialization. Entries with missing or + /// invalid names (not usable as directory names) or missing URLs are excluded and logged. + /// + private List FilterValidEntries(IReadOnlyList entries) + { + var invalidFileNameChars = Path.GetInvalidFileNameChars(); + + var valid = new List(entries.Count); + + foreach (var entry in entries) + { + if (string.IsNullOrWhiteSpace(entry.Name)) + { + LogIndexEntrySkipped(this._logger, "(unnamed)", "archive entry missing required 'name' field"); + continue; + } + + if (!IsValidDirectoryName(entry.Name!, invalidFileNameChars)) + { + LogIndexEntrySkipped(this._logger, entry.Name!, "name contains invalid characters for a directory name"); + continue; + } + + if (string.IsNullOrWhiteSpace(entry.Url)) + { + LogIndexEntrySkipped(this._logger, entry.Name!, "missing required 'url' field"); + continue; + } + + valid.Add(entry); + } + + return valid; + + static bool IsValidDirectoryName(string value, char[] invalidChars) + { + foreach (char c in value) + { + if (Array.IndexOf(invalidChars, c) >= 0 || c == Path.DirectorySeparatorChar || c == Path.AltDirectorySeparatorChar) + { + return false; + } + } + + // Names consisting solely of dots or whitespace are invalid directory names. + string trimmed = value.Trim().Trim('.'); + return trimmed.Length > 0; + } + } + + /// + /// Replaces control characters in a file-system path with ? so the path is safe to include + /// in log messages without risking terminal-escape injection. + /// + private static string SanitizePathForLog(string path) + { + char[]? chars = null; + for (int i = 0; i < path.Length; i++) + { + if (char.IsControl(path[i])) + { + chars ??= path.ToCharArray(); + chars[i] = '?'; + } + } + + return chars is null ? path : new string(chars); + } + + /// + /// Attempts to recursively delete a skill directory. + /// + private bool TryDeleteSkillDirectory(string skillName, string directory) + { + try + { + if (Directory.Exists(directory)) + { + Directory.Delete(directory, recursive: true); + } + + return true; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + LogArchiveSkillCleanupFailed(this._logger, skillName, ex); + return false; + } + } + + [LoggerMessage(LogLevel.Debug, "Skipping skill index entry '{SkillName}': {Reason}")] + private static partial void LogIndexEntrySkipped(ILogger logger, string skillName, string reason); + + [LoggerMessage(LogLevel.Debug, "Extracted archive skill '{SkillName}' to {TargetDirectory}")] + private static partial void LogArchiveExtracted(ILogger logger, string skillName, string targetDirectory); + + [LoggerMessage(LogLevel.Debug, "Pruned stale archive skill '{SkillName}' at {TargetDirectory}")] + private static partial void LogArchiveSkillPruned(ILogger logger, string skillName, string targetDirectory); + + [LoggerMessage(LogLevel.Warning, "Failed to prune stale archive skill '{SkillName}'.")] + private static partial void LogArchiveSkillPruneFailed(ILogger logger, string skillName, Exception exception); + + [LoggerMessage(LogLevel.Warning, "Failed to clean existing archive skill directory for '{SkillName}'.")] + private static partial void LogArchiveSkillCleanupFailed(ILogger logger, string skillName, Exception exception); + + [LoggerMessage(LogLevel.Warning, "Failed to read archive resource for skill '{SkillName}'.")] + private static partial void LogArchiveReadFailed(ILogger logger, string skillName, Exception exception); + + [LoggerMessage(LogLevel.Warning, "Failed to decode archive resource for skill '{SkillName}'.")] + private static partial void LogArchiveDecodeFailed(ILogger logger, string skillName, Exception exception); + + [LoggerMessage(LogLevel.Warning, "Failed to extract archive for skill '{SkillName}'.")] + private static partial void LogArchiveExtractFailed(ILogger logger, string skillName, Exception exception); +} diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs new file mode 100644 index 00000000000..07d2c5f858c --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs @@ -0,0 +1,21 @@ +// Copyright (c) Microsoft. All rights reserved. + +namespace Microsoft.Agents.AI; + +/// +/// The archive container formats supported by . +/// +internal enum ArchiveFormat +{ + /// The format could not be determined. + Unknown, + + /// A ZIP archive. + Zip, + + /// An uncompressed TAR archive. + Tar, + + /// A gzip-compressed TAR archive (.tar.gz/.tgz). + TarGz, +} diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/IMcpSkillEntryLoader.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/IMcpSkillEntryLoader.cs new file mode 100644 index 00000000000..34b678165f5 --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/IMcpSkillEntryLoader.cs @@ -0,0 +1,35 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +namespace Microsoft.Agents.AI; + +/// +/// Loads instances from skill://index.json entries of a single +/// distribution (e.g. skill-md or archive). +/// +/// +/// Implementations receive all index entries of their type as a batch so that types whose +/// materialization spans the whole set (e.g. archive, which reconciles and prunes a shared +/// on-disk directory) can reason about every advertised entry at once. The batch may be empty: every +/// registered loader is invoked even when the server advertises no entries of its type, so that +/// type-wide lifecycle work (such as pruning leftover archive directories) still runs. +/// +internal interface IMcpSkillEntryLoader +{ + /// + /// Gets the index entry type this loader handles (matched case-insensitively). + /// + string EntryType { get; } + + /// + /// Loads the skills for every supplied entry. Entries that are invalid or fail to load are + /// skipped (and logged) rather than throwing; the returned list contains only the skills that + /// loaded successfully. + /// + /// The index entries of this loader's . May be empty. + /// A token to cancel the operation. + Task> LoadAsync(IReadOnlyList entries, CancellationToken cancellationToken); +} diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs new file mode 100644 index 00000000000..bf910478485 --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs @@ -0,0 +1,80 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using ModelContextProtocol.Client; + +namespace Microsoft.Agents.AI; + +/// +/// Loads skill-md index entries: each entry's SKILL.md and sibling resources are +/// fetched on demand from the MCP server via an . +/// +internal sealed partial class SkillMdEntryLoader : IMcpSkillEntryLoader +{ + private readonly McpClient _client; + private readonly ILogger _logger; + + public SkillMdEntryLoader(McpClient client, ILoggerFactory loggerFactory) + { + this._client = client; + this._logger = loggerFactory.CreateLogger(); + } + + /// + public string EntryType => "skill-md"; + + /// + public Task> LoadAsync(IReadOnlyList entries, CancellationToken cancellationToken) + { + var skills = new List(); + + foreach (var entry in entries) + { + if (this.TryLoadSkillMdEntry(entry, out AgentSkill? skill)) + { + skills.Add(skill); + } + } + + return Task.FromResult>(skills); + } + + private bool TryLoadSkillMdEntry(McpSkillIndexEntry entry, [NotNullWhen(true)] out AgentSkill? skill) + { + skill = null; + + if (string.IsNullOrWhiteSpace(entry.Url)) + { + LogIndexEntrySkipped(this._logger, entry.Name ?? "(unnamed)", "missing required 'url' field"); + return false; + } + + AgentSkillFrontmatter frontmatter; + try + { + frontmatter = new AgentSkillFrontmatter(entry.Name!, entry.Description!); + } + catch (ArgumentException ex) + { + LogIndexEntrySkipped(this._logger, entry.Name ?? "(unnamed)", $"invalid metadata: {ex.Message}"); + return false; + } + + skill = new AgentMcpSkill(frontmatter, entry.Url!, this._client); + + LogSkillLoaded(this._logger, frontmatter.Name); + + return true; + } + + [LoggerMessage(LogLevel.Information, "Loaded MCP skill: {SkillName}")] + private static partial void LogSkillLoaded(ILogger logger, string skillName); + + [LoggerMessage(LogLevel.Debug, "Skipping skill index entry '{SkillName}': {Reason}")] + private static partial void LogIndexEntrySkipped(ILogger logger, string skillName, string reason); +} diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProviderBuilder.cs b/dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProviderBuilder.cs index e49c620187a..c07174b8785 100644 --- a/dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProviderBuilder.cs +++ b/dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProviderBuilder.cs @@ -131,6 +131,20 @@ public AgentSkillsProviderBuilder UseSource(AgentSkillsSource source) return this; } + /// + /// Adds a custom skill source created by a factory that receives the builder's logger factory + /// at build time. Use this overload when the source needs logging and should not require the + /// caller to pass an explicitly. + /// + /// A factory that creates the skill source given an optional logger factory. + /// This builder instance for chaining. + public AgentSkillsProviderBuilder UseSource(Func factory) + { + _ = Throw.IfNull(factory); + this._sourceFactories.Add((_, loggerFactory) => factory(loggerFactory)); + return this; + } + /// /// Sets a custom system prompt template. /// diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs index 54a5dec10cc..36d2be3ec84 100644 --- a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs @@ -27,7 +27,7 @@ namespace Microsoft.Agents.AI; /// Resource and script paths are checked against path traversal and symlink escape attacks. /// [Experimental(DiagnosticIds.Experiments.AgentsAIExperiments)] -internal sealed partial class AgentFileSkillsSource : AgentSkillsSource +public sealed partial class AgentFileSkillsSource : AgentSkillsSource { private const string SkillFileName = "SKILL.md"; private const int DefaultSearchDepth = 2; diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs new file mode 100644 index 00000000000..2e195a7008f --- /dev/null +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs @@ -0,0 +1,693 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Formats.Tar; +using System.IO; +using System.IO.Compression; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using ModelContextProtocol.Protocol; +using ModelContextProtocol.Server; + +namespace Microsoft.Agents.AI.Skills.Mcp.UnitTests; + +/// +/// Unit tests for archive-distributed skill handling in . +/// +public sealed class AgentMcpSkillsSourceArchiveTests : IDisposable +{ + private const string ArchivedSkillMd = """ + --- + name: archived-skill + description: A skill delivered as an archive. + --- + # Archived Skill + + Body from the archive. + """; + + private const string StaleSkillMd = """ + --- + name: stale-skill + description: Left over from a previous session. + --- + Old content. + """; + + private const string SkillAMd = """ + --- + name: skill-a + description: Skill A. + --- + Content A. + """; + + private const string SkillBMd = """ + --- + name: skill-b + description: Skill B. + --- + Content B. + """; + + private readonly string _extractionRoot = + Path.Combine(Path.GetTempPath(), "af-mcp-archive-tests", Guid.NewGuid().ToString("N")); + + private const int ManyFileArchiveFileCount = 60; + + [Fact] + public async Task GetSkillsAsync_ZipArchive_DiscoversSkillAsync() + { + // Arrange + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions { ArchiveSkillsDirectory = this._extractionRoot }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert + var skill = Assert.Single(skills); + Assert.Equal("archived-skill", skill.Frontmatter.Name); + Assert.Equal("A skill delivered as an archive.", skill.Frontmatter.Description); + Assert.Contains("Body from the archive.", await skill.GetContentAsync()); + } + + [Fact] + public async Task GetSkillsAsync_TarGzArchive_DiscoversSkillAsync() + { + // Arrange + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions { ArchiveSkillsDirectory = this._extractionRoot }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert + var skill = Assert.Single(skills); + Assert.Equal("archived-skill", skill.Frontmatter.Name); + Assert.Contains("Body from the archive.", await skill.GetContentAsync()); + } + + [Fact] + public async Task GetSkillsAsync_ArchiveWithScript_SurfacesScriptAsReadableResourceAsync() + { + // Arrange - archive bundles a script file alongside SKILL.md. Over MCP, scripts must never be + // executable; they are surfaced as readable resources only when explicitly included via options. + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions + { + ArchiveSkillsDirectory = this._extractionRoot, + ArchiveResourceExtensions = [".md", ".json", ".yaml", ".yml", ".csv", ".xml", ".txt", ".py"], + }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skill = Assert.Single(await source.GetSkillsAsync()); + var resource = await skill.GetResourceAsync("scripts/run.py"); + + // Assert - the .py file is readable as a resource (not an executable script). + Assert.NotNull(resource); + var content = await resource!.ReadAsync(); + Assert.Equal("print('hello')", content); + } + + [Fact] + public async Task GetSkillsAsync_TwoSourcesWithSeparateDirectories_DoNotCollideAsync() + { + // Arrange - two servers publish an archive skill with the SAME name but different content. + // Pointing each source at its own directory keeps their extracted content separate. + await using var serverA = new InMemoryMcpServer(builder => builder.WithResources()); + await using var clientA = await serverA.CreateClientAsync(); + await using var serverB = new InMemoryMcpServer(builder => builder.WithResources()); + await using var clientB = await serverB.CreateClientAsync(); + + var sourceA = new AgentMcpSkillsSource(clientA, new() { ArchiveSkillsDirectory = Path.Combine(this._extractionRoot, "a") }); + var sourceB = new AgentMcpSkillsSource(clientB, new() { ArchiveSkillsDirectory = Path.Combine(this._extractionRoot, "b") }); + + // Act + var skillA = Assert.Single(await sourceA.GetSkillsAsync()); + var skillB = Assert.Single(await sourceB.GetSkillsAsync()); + + // Assert - each source kept its own content despite the shared skill name. + Assert.Equal("shared-skill", skillA.Frontmatter.Name); + Assert.Equal("shared-skill", skillB.Frontmatter.Name); + Assert.Contains("Content from server A.", await skillA.GetContentAsync()); + Assert.Contains("Content from server B.", await skillB.GetContentAsync()); + } + + [Fact] + public async Task GetSkillsAsync_FirstRun_PrunesLeftoverSkillDirectoryAsync() + { + // Arrange - a leftover skill directory from a previous session sits in the provided directory. + string staleDir = Path.Combine(this._extractionRoot, "stale-skill"); + Directory.CreateDirectory(staleDir); + await File.WriteAllTextAsync(Path.Combine(staleDir, "SKILL.md"), StaleSkillMd); + + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions { ArchiveSkillsDirectory = this._extractionRoot }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert - the leftover directory is pruned and only the advertised skill remains. + Assert.False(Directory.Exists(staleDir)); + var skill = Assert.Single(skills); + Assert.Equal("archived-skill", skill.Frontmatter.Name); + } + + [Fact] + public async Task GetSkillsAsync_SkillNoLongerAdvertised_IsPrunedAsync() + { + // Arrange - an earlier run extracts skill-a and skill-b into the shared directory. + await using var fullServer = new InMemoryMcpServer(builder => builder.WithResources()); + await using var fullClient = await fullServer.CreateClientAsync(); + var firstSource = new AgentMcpSkillsSource(fullClient, new() { ArchiveSkillsDirectory = this._extractionRoot }); + var firstSkills = await firstSource.GetSkillsAsync(); + Assert.Equal(2, firstSkills.Count); + Assert.True(Directory.Exists(Path.Combine(this._extractionRoot, "skill-b"))); + + // Act - a later run sees only skill-a. + await using var partialServer = new InMemoryMcpServer(builder => builder.WithResources()); + await using var partialClient = await partialServer.CreateClientAsync(); + var secondSource = new AgentMcpSkillsSource(partialClient, new() { ArchiveSkillsDirectory = this._extractionRoot }); + var secondSkills = await secondSource.GetSkillsAsync(); + + // Assert - skill-b's directory was pruned; only skill-a remains. + var skill = Assert.Single(secondSkills); + Assert.Equal("skill-a", skill.Frontmatter.Name); + Assert.False(Directory.Exists(Path.Combine(this._extractionRoot, "skill-b"))); + Assert.True(Directory.Exists(Path.Combine(this._extractionRoot, "skill-a"))); + } + + [Fact] + public async Task GetSkillsAsync_ServerListsNoArchives_PrunesLeftoversAsync() + { + // Arrange - a leftover skill directory exists but the server advertises no archive skills. + string staleDir = Path.Combine(this._extractionRoot, "stale-skill"); + Directory.CreateDirectory(staleDir); + await File.WriteAllTextAsync(Path.Combine(staleDir, "SKILL.md"), StaleSkillMd); + + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions { ArchiveSkillsDirectory = this._extractionRoot }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert - the leftover directory is pruned and no skills are returned. + Assert.Empty(skills); + Assert.False(Directory.Exists(staleDir)); + } + + [Fact] + public async Task GetSkillsAsync_SecondDiscovery_ReExtractsContentAsync() + { + // Arrange - a first run extracts server A's content into the shared directory. + await using (var serverA = new InMemoryMcpServer(builder => builder.WithResources())) + await using (var clientA = await serverA.CreateClientAsync()) + { + var firstSource = new AgentMcpSkillsSource(clientA, new() { ArchiveSkillsDirectory = this._extractionRoot }); + Assert.Single(await firstSource.GetSkillsAsync()); + } + + // Act - a second run over the same directory re-extracts server B's content. + await using var serverB = new InMemoryMcpServer(builder => builder.WithResources()); + await using var clientB = await serverB.CreateClientAsync(); + var source = new AgentMcpSkillsSource(clientB, new() { ArchiveSkillsDirectory = this._extractionRoot }); + var skill = Assert.Single(await source.GetSkillsAsync()); + + // Assert - the content was replaced with server B's. + Assert.Contains("Content from server B.", await skill.GetContentAsync()); + } + + [Fact] + public async Task GetSkillsAsync_PreExtractionCleanupFails_DoesNotDiscoverStaleSkillAsync() + { + if (!OperatingSystem.IsWindows()) + { + return; + } + + // Arrange - a stale skill directory contains a locked file, causing recursive deletion to fail. + string skillDirectory = Path.Combine(this._extractionRoot, "shared-skill"); + Directory.CreateDirectory(skillDirectory); + await File.WriteAllTextAsync(Path.Combine(skillDirectory, "SKILL.md"), """ + --- + name: shared-skill + description: Shared. + --- + Stale content. + """); + + string lockedResourcePath = Path.Combine(skillDirectory, "locked.txt"); + await using var lockedResource = new FileStream(lockedResourcePath, FileMode.Create, FileAccess.ReadWrite, FileShare.None); + + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var source = new AgentMcpSkillsSource(client, new() { ArchiveSkillsDirectory = this._extractionRoot }); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert - failed cleanup prevents the stale directory from being proxied to AgentFileSkillsSource. + Assert.Empty(skills); + Assert.True(File.Exists(lockedResourcePath)); + } + + [Fact] + public void Extract_ZipSlipEntry_DoesNotEscapeTargetDirectory() + { + // Arrange - a malicious zip whose entry traverses out of the target directory. + byte[] zip = BuildZip(("../escaped.txt", "pwned"), ("SKILL.md", ArchivedSkillMd)); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act + AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target); + + // Assert - the traversal entry was skipped; only the safe entry was written. + Assert.False(File.Exists(Path.Combine(this._extractionRoot, "escaped.txt"))); + Assert.True(File.Exists(Path.Combine(target, "SKILL.md"))); + } + + [Fact] + public void Extract_CaseVariantZipSlipEntry_DoesNotEscapeTargetDirectory() + { + if (OperatingSystem.IsWindows()) + { + return; + } + + // Arrange - on case-sensitive file systems, "skill" is a sibling of "Skill", not the target. + byte[] zip = BuildZip(("../skill/escaped.txt", "pwned"), ("SKILL.md", ArchivedSkillMd)); + string target = Path.Combine(this._extractionRoot, "Skill"); + + // Act + AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target); + + // Assert - the case-variant traversal entry was skipped; only the safe entry was written. + Assert.False(File.Exists(Path.Combine(this._extractionRoot, "skill", "escaped.txt"))); + Assert.True(File.Exists(Path.Combine(target, "SKILL.md"))); + } + + [Fact] + public void Extract_ArchiveExceedsDefaultFileCount_Throws() + { + // Arrange - more files than the default cap. + var entries = Enumerable.Range(0, AgentMcpSkillArchiveExtractor.DefaultMaxFileCount + 1) + .Select(i => ($"file{i}.txt", "x")) + .ToArray(); + byte[] zip = BuildZip(entries); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act / Assert + Assert.Throws( + () => AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target)); + } + + [Fact] + public void Extract_ArchiveExceedsDefaultUncompressedSize_Throws() + { + // Arrange - a single file larger than the default uncompressed budget (1 MB). + string oversized = new('x', (int)AgentMcpSkillArchiveExtractor.DefaultMaxUncompressedSizeBytes + 1); + byte[] zip = BuildZip(("SKILL.md", oversized)); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act / Assert + Assert.Throws( + () => AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target)); + } + + [Fact] + public void Extract_TarGzExceedsUncompressedSize_Throws() + { + // Arrange - a gzip-compressed tar whose expansion exceeds the default budget. The ZIP pre-gate + // does not apply here, so this exercises the authoritative streaming cap (CopyWithLimit). + string oversized = new('x', (int)AgentMcpSkillArchiveExtractor.DefaultMaxUncompressedSizeBytes + 1); + byte[] tarGz = BuildTarGz(("SKILL.md", oversized)); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act / Assert + Assert.Throws( + () => AgentMcpSkillArchiveExtractor.Extract(tarGz, ArchiveFormat.TarGz, target)); + } + + [Fact] + public void Extract_TarWithLinkEntries_SkipsLinksAndExtractsRegularFiles() + { + // Arrange - a tar.gz containing symbolic-link and hard-link entries whose targets escape the + // target directory, alongside a regular file. Link entries must be skipped so an archive cannot + // create links that point outside the target directory. + byte[] tarGz = BuildTarGzFromEntries( + new PaxTarEntry(TarEntryType.SymbolicLink, "evil-symlink") { LinkName = "../../escaped.txt" }, + new PaxTarEntry(TarEntryType.HardLink, "evil-hardlink") { LinkName = "../../escaped.txt" }, + new PaxTarEntry(TarEntryType.RegularFile, "SKILL.md") + { + DataStream = new MemoryStream(Encoding.UTF8.GetBytes(ArchivedSkillMd)), + }); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act + AgentMcpSkillArchiveExtractor.Extract(tarGz, ArchiveFormat.TarGz, target); + + // Assert - only the regular file is materialized; neither link entry is written. + Assert.True(File.Exists(Path.Combine(target, "SKILL.md"))); + Assert.False(File.Exists(Path.Combine(target, "evil-symlink"))); + Assert.False(File.Exists(Path.Combine(target, "evil-hardlink"))); + Assert.Single(Directory.GetFileSystemEntries(target)); + } + + [Fact] + public void Extract_WithinDefaultLimits_Succeeds() + { + // Arrange - a typical small archive that is comfortably within the default limits. + byte[] zip = BuildZip(("SKILL.md", ArchivedSkillMd), ("reference.md", "Some reference content.")); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act + AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target); + + // Assert + Assert.True(File.Exists(Path.Combine(target, "SKILL.md"))); + Assert.True(File.Exists(Path.Combine(target, "reference.md"))); + } + + [Fact] + public void Extract_RaisedLimits_AllowsLargerArchive() + { + // Arrange - an archive that exceeds the default file count but fits within raised limits. + var entries = Enumerable.Range(0, AgentMcpSkillArchiveExtractor.DefaultMaxFileCount + 1) + .Select(i => ($"file{i}.txt", "x")) + .ToArray(); + byte[] zip = BuildZip(entries); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act + AgentMcpSkillArchiveExtractor.Extract( + zip, + ArchiveFormat.Zip, + target, + maxFileCount: entries.Length + 1); + + // Assert - every entry was extracted. + Assert.Equal(entries.Length, Directory.GetFiles(target).Length); + } + + [Fact] + public async Task GetSkillsAsync_ArchiveExceedsDefaultFileCount_SkillSkippedAsync() + { + // Arrange - the archive has more files than the default cap, so extraction fails and the skill is skipped. + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var source = new AgentMcpSkillsSource(client, new() { ArchiveSkillsDirectory = this._extractionRoot }); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert + Assert.Empty(skills); + } + + [Fact] + public async Task GetSkillsAsync_ArchiveExceedsConfiguredArchiveSize_SkillSkippedAsync() + { + // Arrange - the valid archive is larger than the configured downloaded-archive byte cap. + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions + { + ArchiveSkillsDirectory = this._extractionRoot, + ArchiveMaxSizeBytes = 1, + }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skills = await source.GetSkillsAsync(); + + // Assert + Assert.Empty(skills); + } + + [Fact] + public async Task GetSkillsAsync_RaisedFileCountOption_LoadsLargerArchiveAsync() + { + // Arrange - raising ArchiveMaxFileCount lets the larger archive through. + await using var server = new InMemoryMcpServer(builder => builder.WithResources()); + await using var client = await server.CreateClientAsync(); + var options = new AgentMcpSkillsSourceOptions + { + ArchiveSkillsDirectory = this._extractionRoot, + ArchiveMaxFileCount = ManyFileArchiveFileCount + 1, + }; + var source = new AgentMcpSkillsSource(client, options); + + // Act + var skill = Assert.Single(await source.GetSkillsAsync()); + + // Assert + Assert.Equal("archived-skill", skill.Frontmatter.Name); + } + + public void Dispose() + { + try + { + if (Directory.Exists(this._extractionRoot)) + { + Directory.Delete(this._extractionRoot, recursive: true); + } + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // Best-effort cleanup. + } + } + + private static byte[] BuildZip(params (string Path, string Content)[] entries) + { + using var ms = new MemoryStream(); + using (var archive = new ZipArchive(ms, ZipArchiveMode.Create, leaveOpen: true)) + { + foreach (var (path, content) in entries) + { + ZipArchiveEntry entry = archive.CreateEntry(path); + using Stream stream = entry.Open(); + byte[] bytes = Encoding.UTF8.GetBytes(content); + stream.Write(bytes, 0, bytes.Length); + } + } + + return ms.ToArray(); + } + + private static byte[] BuildTarGz(params (string Path, string Content)[] entries) + { + using var ms = new MemoryStream(); + using (var gzip = new GZipStream(ms, CompressionMode.Compress, leaveOpen: true)) + using (var writer = new TarWriter(gzip, leaveOpen: true)) + { + foreach (var (path, content) in entries) + { + var entry = new PaxTarEntry(TarEntryType.RegularFile, path) + { + DataStream = new MemoryStream(Encoding.UTF8.GetBytes(content)), + }; + writer.WriteEntry(entry); + } + } + + return ms.ToArray(); + } + + private static byte[] BuildTarGzFromEntries(params TarEntry[] entries) + { + using var ms = new MemoryStream(); + using (var gzip = new GZipStream(ms, CompressionMode.Compress, leaveOpen: true)) + using (var writer = new TarWriter(gzip, leaveOpen: true)) + { + foreach (var entry in entries) + { + writer.WriteEntry(entry); + } + } + + return ms.ToArray(); + } + + private static string ArchiveIndex(string skillName, string url) => $$""" + { + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { + "name": "{{skillName}}", + "type": "archive", + "description": "A skill delivered as an archive.", + "url": "{{url}}" + } + ] + } + """; + + #region Resource classes (registered with the MCP server via WithResources) + +#pragma warning disable CA1812 + + [McpServerResourceType] + private sealed class ZipArchiveServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("archived-skill", "skill://archives/archived-skill.zip"); + + [McpServerResource(UriTemplate = "skill://archives/archived-skill.zip", Name = "archive", MimeType = "application/zip")] + public static BlobResourceContents Archive() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", ArchivedSkillMd)), + "skill://archives/archived-skill.zip", + "application/zip"); + } + + [McpServerResourceType] + private sealed class TarGzArchiveServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("archived-skill", "skill://archives/archived-skill.tar.gz"); + + [McpServerResource(UriTemplate = "skill://archives/archived-skill.tar.gz", Name = "archive", MimeType = "application/gzip")] + public static BlobResourceContents Archive() => BlobResourceContents.FromBytes( + BuildTarGz(("SKILL.md", ArchivedSkillMd)), + "skill://archives/archived-skill.tar.gz", + "application/gzip"); + } + + [McpServerResourceType] + private sealed class ZipWithScriptServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("archived-skill", "skill://archives/archived-skill.zip"); + + [McpServerResource(UriTemplate = "skill://archives/archived-skill.zip", Name = "archive", MimeType = "application/zip")] + public static BlobResourceContents Archive() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", ArchivedSkillMd), ("scripts/run.py", "print('hello')")), + "skill://archives/archived-skill.zip", + "application/zip"); + } + + [McpServerResourceType] + private sealed class ManyFileArchiveServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("archived-skill", "skill://archives/archived-skill.zip"); + + [McpServerResource(UriTemplate = "skill://archives/archived-skill.zip", Name = "archive", MimeType = "application/zip")] + public static BlobResourceContents Archive() + { + var entries = new List<(string Path, string Content)> { ("SKILL.md", ArchivedSkillMd) }; + for (int i = 0; i < ManyFileArchiveFileCount - 1; i++) + { + entries.Add(($"reference/file{i}.txt", "x")); + } + + return BlobResourceContents.FromBytes( + BuildZip(entries.ToArray()), + "skill://archives/archived-skill.zip", + "application/zip"); + } + } + + [McpServerResourceType] + private sealed class SharedNameServerA + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("shared-skill", "skill://archives/shared-skill.zip"); + + [McpServerResource(UriTemplate = "skill://archives/shared-skill.zip", Name = "archive", MimeType = "application/zip")] + public static BlobResourceContents Archive() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", """ + --- + name: shared-skill + description: Shared. + --- + Content from server A. + """)), + "skill://archives/shared-skill.zip", + "application/zip"); + } + + [McpServerResourceType] + private sealed class SharedNameServerB + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("shared-skill", "skill://archives/shared-skill.zip"); + + [McpServerResource(UriTemplate = "skill://archives/shared-skill.zip", Name = "archive", MimeType = "application/zip")] + public static BlobResourceContents Archive() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", """ + --- + name: shared-skill + description: Shared. + --- + Content from server B. + """)), + "skill://archives/shared-skill.zip", + "application/zip"); + } + + [McpServerResourceType] + private sealed class TwoSkillServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => """ + { + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { "name": "skill-a", "type": "archive", "description": "Skill A.", "url": "skill://archives/skill-a.zip" }, + { "name": "skill-b", "type": "archive", "description": "Skill B.", "url": "skill://archives/skill-b.zip" } + ] + } + """; + + [McpServerResource(UriTemplate = "skill://archives/skill-a.zip", Name = "skill-a", MimeType = "application/zip")] + public static BlobResourceContents SkillA() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", SkillAMd)), "skill://archives/skill-a.zip", "application/zip"); + + [McpServerResource(UriTemplate = "skill://archives/skill-b.zip", Name = "skill-b", MimeType = "application/zip")] + public static BlobResourceContents SkillB() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", SkillBMd)), "skill://archives/skill-b.zip", "application/zip"); + } + + [McpServerResourceType] + private sealed class OneSkillServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => ArchiveIndex("skill-a", "skill://archives/skill-a.zip"); + + [McpServerResource(UriTemplate = "skill://archives/skill-a.zip", Name = "skill-a", MimeType = "application/zip")] + public static BlobResourceContents SkillA() => BlobResourceContents.FromBytes( + BuildZip(("SKILL.md", SkillAMd)), "skill://archives/skill-a.zip", "application/zip"); + } + + [McpServerResourceType] + private sealed class NoArchiveServer + { + [McpServerResource(UriTemplate = "skill://index.json", Name = "index", MimeType = "application/json")] + public static string Index() => """ + { + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [] + } + """; + } + +#pragma warning restore CA1812 + + #endregion +} diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs index bf5f52758f7..891f2a726b4 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs @@ -40,7 +40,7 @@ Body content here. [Fact] public async Task GetSkillsAsync_IndexBasedDiscovery_ReturnsSkillAsync() { - // Arrange — server exposes both skill://index.json and the skill itself. + // Arrange - server exposes both skill://index.json and the skill itself. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); await using var client = await server.CreateClientAsync(); @@ -49,7 +49,7 @@ public async Task GetSkillsAsync_IndexBasedDiscovery_ReturnsSkillAsync() // Act var skills = await source.GetSkillsAsync(); - // Assert — frontmatter comes from index; Content is the actual SKILL.md body from the server. + // Assert - frontmatter comes from index; Content is the actual SKILL.md body from the server. var skill = Assert.Single(skills); Assert.Equal("unit-converter", skill.Frontmatter.Name); Assert.Equal("Convert between common units.", skill.Frontmatter.Description); @@ -63,7 +63,7 @@ public async Task GetSkillsAsync_IndexBasedDiscovery_ReturnsSkillAsync() [Fact] public async Task GetSkillsAsync_NoIndex_ReturnsEmptyAsync() { - // Arrange — server only exposes SKILL.md, no skill://index.json. + // Arrange - server only exposes SKILL.md, no skill://index.json. // Per SEP-2640, discovery requires the index document; without it, no skills are surfaced. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); @@ -80,7 +80,7 @@ public async Task GetSkillsAsync_NoIndex_ReturnsEmptyAsync() [Fact] public async Task GetResourceAsync_SiblingText_ReturnsContentAsync() { - // Arrange — server exposes index, SKILL.md, and a sibling reference file. + // Arrange - server exposes index, SKILL.md, and a sibling reference file. // The skill reads the sibling on demand via GetResourceAsync. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); @@ -100,7 +100,7 @@ public async Task GetResourceAsync_SiblingText_ReturnsContentAsync() [Fact] public async Task GetResourceAsync_SiblingBinary_ReturnsDataContentAsync() { - // Arrange — server exposes index, SKILL.md, and a binary sibling. + // Arrange - server exposes index, SKILL.md, and a binary sibling. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); await using var client = await server.CreateClientAsync(); @@ -121,7 +121,7 @@ public async Task GetResourceAsync_SiblingBinary_ReturnsDataContentAsync() [Fact] public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() { - // Arrange — index advertises a skill, but no sibling resource exists. + // Arrange - index advertises a skill, but no sibling resource exists. // GetResourceAsync eagerly fetches from the MCP server; a non-existent // resource causes the server to return an error, so null is returned. await using var server = new InMemoryMcpServer(builder => @@ -133,7 +133,7 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() var skill = Assert.Single(await source.GetSkillsAsync()); var resource = await skill.GetResourceAsync("references/does-not-exist.md"); - // Assert — resource does not exist on the server, so null is returned + // Assert - resource does not exist on the server, so null is returned Assert.Null(resource); } @@ -143,7 +143,7 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("..")] public async Task GetResourceAsync_PathTraversalName_ReturnsNullAsync(string name) { - // Arrange — '..' segments result in URIs that don't match any server resource. + // Arrange - '..' segments result in URIs that don't match any server resource. // The MCP server returns an error for unknown URIs, so GetResourceAsync returns null. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); @@ -154,14 +154,14 @@ public async Task GetResourceAsync_PathTraversalName_ReturnsNullAsync(string nam var skill = Assert.Single(await source.GetSkillsAsync()); var resource = await skill.GetResourceAsync(name); - // Assert — resource does not exist on the server, so null is returned + // Assert - resource does not exist on the server, so null is returned Assert.Null(resource); } [Fact] public async Task GetSkillsAsync_DoesNotReadSkillMdAsync() { - // Arrange — index points to a non-existent SKILL.md URI. Because the source builds + // Arrange - index points to a non-existent SKILL.md URI. Because the source builds // skills from index info only, discovery still succeeds. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); @@ -171,7 +171,7 @@ public async Task GetSkillsAsync_DoesNotReadSkillMdAsync() // Act var skills = await source.GetSkillsAsync(); - // Assert — discovery succeeds from index alone. + // Assert - discovery succeeds from index alone. var skill = Assert.Single(skills); Assert.Equal("unit-converter", skill.Frontmatter.Name); } @@ -179,7 +179,7 @@ public async Task GetSkillsAsync_DoesNotReadSkillMdAsync() [Fact] public async Task GetSkillsAsync_IndexEntryWithInvalidName_IsSkippedAsync() { - // Arrange — index entry has an invalid (uppercase) name, which AgentSkillFrontmatter rejects. + // Arrange - index entry has an invalid (uppercase) name, which AgentSkillFrontmatter rejects. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); await using var client = await server.CreateClientAsync(); @@ -195,7 +195,7 @@ public async Task GetSkillsAsync_IndexEntryWithInvalidName_IsSkippedAsync() [Fact] public async Task GetSkillsAsync_IndexEntryWithMissingRequiredFields_IsSkippedAsync() { - // Arrange — index entry is missing the required description and url fields. + // Arrange - index entry is missing the required description and url fields. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); await using var client = await server.CreateClientAsync(); @@ -209,9 +209,10 @@ public async Task GetSkillsAsync_IndexEntryWithMissingRequiredFields_IsSkippedAs } [Fact] - public async Task GetSkillsAsync_IndexEntryWithUnsupportedType_IsSkippedAsync() + public async Task GetSkillsAsync_ArchiveEntryWithUnreadableResource_IsSkippedAsync() { - // Arrange — index has an "archive" entry, which this source does not support. + // Arrange - index has an "archive" entry, but the referenced archive resource does not + // exist on the server, so reading it fails and the entry is skipped gracefully. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); await using var client = await server.CreateClientAsync(); @@ -227,7 +228,7 @@ public async Task GetSkillsAsync_IndexEntryWithUnsupportedType_IsSkippedAsync() [Fact] public async Task GetSkillsAsync_IndexEntryWithTemplateType_IsSkippedAsync() { - // Arrange — index has an "mcp-resource-template" entry (parameterized skill namespace). + // Arrange - index has an "mcp-resource-template" entry (parameterized skill namespace). // The current source skips template entries; they require user input to materialize. await using var server = new InMemoryMcpServer(builder => builder.WithResources()); @@ -243,7 +244,7 @@ public async Task GetSkillsAsync_IndexEntryWithTemplateType_IsSkippedAsync() #region Resource classes (registered with the MCP server via WithResources) - // CA1812 flags these classes as "never instantiated", which is technically correct — + // CA1812 flags these classes as "never instantiated", which is technically correct - // they are never constructed because they only contain static methods (e.g. `public static string Index()`). // The MCP framework discovers and invokes these static methods via the [McpServerResourceType] and // [McpServerResource] attributes registered through WithResources(), without ever creating an instance. diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentSkillsProviderBuilderTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentSkillsProviderBuilderTests.cs index 85335256a70..a95e8acc15d 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentSkillsProviderBuilderTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentSkillsProviderBuilderTests.cs @@ -55,7 +55,7 @@ public void UseSource_NullSource_ThrowsArgumentNullException() var builder = new AgentSkillsProviderBuilder(); // Act & Assert - Assert.Throws(() => builder.UseSource(null!)); + Assert.Throws(() => builder.UseSource((AgentSkillsSource)null!)); } [Fact]