13.4: Load Aspire skills catalog from bundle manifest

src/Aspire.Cli/Agents/AspireSkills/AspireSkillsBundle.cs

@@ -3,6 +3,7 @@
 
 using System.Globalization;
 using System.Security.Cryptography;
+using System.Text;
 using Aspire.Cli.Utils;
 using Semver;
 
@@ -41,13 +42,24 @@ public static async Task<AspireSkillsBundle> LoadAsync(DirectoryInfo bundleDirec
             bundleDirectory,
             VersionHelper.GetDefaultSdkVersion(),
             VersionHelper.GetDefaultSdkVersion(),
+            skipCompatibilityCheck: false,
             cancellationToken).ConfigureAwait(false);
     }
 
+    internal static Task<AspireSkillsBundle> LoadAsync(
+        DirectoryInfo bundleDirectory,
+        string currentCliVersion,
+        string currentSdkVersion,
+        CancellationToken cancellationToken)
+    {
+        return LoadAsync(bundleDirectory, currentCliVersion, currentSdkVersion, skipCompatibilityCheck: false, cancellationToken);
+    }
+
     internal static async Task<AspireSkillsBundle> LoadAsync(
         DirectoryInfo bundleDirectory,
         string currentCliVersion,
         string currentSdkVersion,
+        bool skipCompatibilityCheck,
         CancellationToken cancellationToken)
     {
         ArgumentNullException.ThrowIfNull(bundleDirectory);
@@ -71,7 +83,7 @@ internal static async Task<AspireSkillsBundle> LoadAsync(
             throw new InvalidOperationException("Aspire skills bundle manifest is empty or invalid.");
         }
 
-        ValidateManifest(bundleDirectory, manifest, currentCliVersion, currentSdkVersion);
+        ValidateManifest(bundleDirectory, manifest, currentCliVersion, currentSdkVersion, skipCompatibilityCheck);
 
         return new AspireSkillsBundle(bundleDirectory, manifest);
     }
@@ -107,38 +119,71 @@ public async Task<IReadOnlyList<SkillAssetFile>> GetSkillFilesAsync(SkillDefinit
         return files;
     }
 
+    /// <summary>
+    /// Gets the installable skill definitions declared by the bundle manifest.
+    /// </summary>
+    public IReadOnlyList<SkillDefinition> GetSkillDefinitions()
+    {
+        return _manifest.Skills
+            .Select(static skill => SkillDefinition.CreateAspireSkillsBundle(
+                skill.Name!,
+                skill.Description!,
+                skill.IsDefault,
+                (skill.InstallExcludedRelativePaths ?? []).Select(NormalizeRelativePath).ToArray(),
+                skill.ApplicableLanguages ?? []))
+            .ToList();
+    }
+
     private static void ValidateManifest(
         DirectoryInfo bundleDirectory,
         SkillBundleManifest manifest,
         string currentCliVersion,
-        string currentSdkVersion)
+        string currentSdkVersion,
+        bool skipCompatibilityCheck)
     {
         if (string.IsNullOrWhiteSpace(manifest.Version))
         {
             throw new InvalidOperationException("Aspire skills bundle manifest must specify a version.");
         }
 
-        ValidateCompatibility(manifest.Supports, currentCliVersion, currentSdkVersion);
+        // The bundle's `supports` range gates whether a bundle pulled fresh from GitHub
+        // is allowed at runtime. For bundles we already trust locally — the snapshot
+        // embedded in the CLI binary, and bundles already written to our own cache —
+        // we skip the range check because the CLI's effective version may have moved
+        // past the snapshot's stamped range (e.g., a dogfood build of 13.5.x using a
+        // bundle whose supports declares ">=13.4.0 <13.5.0"). The bundle's `version`
+        // field plus the version-keyed cache directory still gate matching content.
+        if (!skipCompatibilityCheck)
+        {
+            ValidateCompatibility(manifest.Supports, currentCliVersion, currentSdkVersion);
+        }
 
         var skills = manifest.Skills;
         if (skills is not { Length: > 0 })
         {
             throw new InvalidOperationException("Aspire skills bundle manifest must contain at least one skill.");
         }
 
-        var skillNames = new HashSet<string>(StringComparer.Ordinal);
+        var skillNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
         foreach (var skill in skills)
         {
             if (string.IsNullOrWhiteSpace(skill.Name))
             {
                 throw new InvalidOperationException("Aspire skills bundle manifest contains a skill without a name.");
             }
 
+            ValidateSkillName(skill.Name);
+
             if (!skillNames.Add(skill.Name))
             {
                 throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, "Aspire skills bundle manifest contains duplicate skill '{0}'.", skill.Name));
             }
 
+            if (string.IsNullOrWhiteSpace(skill.Description))
+            {
+                throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, "Aspire skills bundle skill '{0}' must specify a description.", skill.Name));
+            }
+
             if (skill.Files is not { Length: > 0 })
             {
                 throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, "Aspire skills bundle skill '{0}' does not contain any files.", skill.Name));
@@ -156,6 +201,62 @@ private static void ValidateManifest(
         }
     }
 
+    // Windows-reserved device names that cannot be used as file/path segments even with extensions.
+    // Listed once here so the rejection set matches the documented Windows filesystem rules:
+    // https://learn.microsoft.com/windows/win32/fileio/naming-a-file#naming-conventions
+    private static readonly HashSet<string> s_reservedDeviceNames = new(StringComparer.OrdinalIgnoreCase)
+    {
+        "CON", "PRN", "AUX", "NUL",
+        "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9",
+        "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9",
+    };
+
+    private static void ValidateSkillName(string skillName)
+    {
+        // Defense-in-depth: although bundle authors are trusted, the skill name becomes a
+        // directory path segment under the user's workspace. Reject anything that could
+        // escape the bundle root, smuggle control characters into logs, or collide with
+        // Windows reserved device names. NFC normalization is checked explicitly because
+        // visually-identical NFD strings would otherwise pass an OrdinalIgnoreCase compare
+        // here but produce a different filesystem name on case-insensitive volumes.
+        if (string.IsNullOrEmpty(skillName) ||
+            Path.IsPathRooted(skillName) ||
+            skillName.IndexOfAny(Path.GetInvalidFileNameChars()) >= 0 ||
+            skillName.Contains('/') ||
+            skillName.Contains('\\') ||
+            skillName is "." or ".." ||
+            ContainsControlCharacter(skillName) ||
+            !string.Equals(skillName, skillName.Normalize(NormalizationForm.FormC), StringComparison.Ordinal) ||
+            IsWindowsReservedDeviceName(skillName))
+        {
+            throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, "Aspire skills bundle skill name '{0}' is not safe.", skillName));
+        }
+
+        static bool ContainsControlCharacter(string value)
+        {
+            foreach (var c in value)
+            {
+                // char.IsControl covers C0 (U+0000-U+001F, U+007F) and C1 (U+0080-U+009F).
+                // Path.GetInvalidFileNameChars() on POSIX only returns { '\0', '/' }, so this is the
+                // portable line of defense against bundle authors injecting U+0001..U+001F or U+0085.
+                if (char.IsControl(c))
+                {
+                    return true;
+                }
+            }
+            return false;
+        }
+
+        static bool IsWindowsReservedDeviceName(string value)
+        {
+            // Windows reserves device names even when a file extension is present (e.g. "CON.md"),
+            // so test the segment up to the first '.'.
+            var dotIndex = value.IndexOf('.');
+            var stem = dotIndex < 0 ? value : value[..dotIndex];
+            return s_reservedDeviceNames.Contains(stem);
+        }
+    }
+
     private static void ValidateFile(DirectoryInfo bundleDirectory, string skillName, SkillBundleFile file)
     {
         var relativePath = NormalizeRelativePath(file.RelativePath);

src/Aspire.Cli/Agents/AspireSkills/AspireSkillsInstaller.cs

@@ -8,9 +8,11 @@
 using System.Net;
 using System.Security.Cryptography;
 using System.Text.Json;
+using Aspire.Cli.Configuration;
 using Aspire.Cli.Interaction;
 using Aspire.Cli.Resources;
 using Aspire.Cli.Telemetry;
+using Aspire.Cli.Utils;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.Logging;
 
@@ -26,6 +28,7 @@ internal sealed class AspireSkillsInstaller(
     IInteractionService interactionService,
     CliExecutionContext executionContext,
     IConfiguration configuration,
+    IFeatures features,
     AspireCliTelemetry telemetry,
     ILogger<AspireSkillsInstaller> logger) : IAspireSkillsInstaller
 {
@@ -72,16 +75,33 @@ private async Task<AspireSkillsInstallResult> InstallCoreAsync(CancellationToken
 
         var validationDisabled = string.Equals(configuration[DisablePackageValidationKey], "true", StringComparison.OrdinalIgnoreCase);
 
-        var githubResult = await InstallFromGitHubAsync(cacheRoot, effectiveVersion, validationDisabled, activity, cancellationToken).ConfigureAwait(false);
-        if (githubResult.Status == AcquisitionStatus.Installed)
+        // The remote fetch path is opt-in. Ship 13.4 with this disabled so users only
+        // get the embedded snapshot (no unattended network call out to GitHub on every
+        // `aspire agent init`). Toggle the feature on to opt in to the GitHub release path,
+        // which still falls back to the embedded snapshot if the network call fails.
+        var remoteFetchEnabled = features.IsFeatureEnabled(
+            KnownFeatures.AspireSkillsRemoteFetchEnabled,
+            KnownFeatures.GetFeatureMetadata(KnownFeatures.AspireSkillsRemoteFetchEnabled)!.DefaultValue);
+        activity?.SetTag("aspire.skills.remote_fetch_enabled", remoteFetchEnabled);
+
+        AcquisitionResult? githubResult = null;
+        if (remoteFetchEnabled)
         {
-            CleanupStaleCacheEntries(cacheRoot, effectiveVersion);
-            return AspireSkillsInstallResult.Installed(githubResult.Bundle!);
-        }
+            githubResult = await InstallFromGitHubAsync(cacheRoot, effectiveVersion, validationDisabled, activity, cancellationToken).ConfigureAwait(false);
+            if (githubResult.Status == AcquisitionStatus.Installed)
+            {
+                CleanupStaleCacheEntries(cacheRoot, effectiveVersion);
+                return AspireSkillsInstallResult.Installed(githubResult.Bundle!);
+            }
 
-        if (githubResult.Status == AcquisitionStatus.Failed)
+            if (githubResult.Status == AcquisitionStatus.Failed)
+            {
+                logger.LogDebug("Aspire skills GitHub acquisition failed for version {Version}; falling back to embedded snapshot. Failure: {Failure}", effectiveVersion, githubResult.Message);
+            }
+        }
+        else
         {
-            logger.LogDebug("Aspire skills GitHub acquisition failed for version {Version}; falling back to embedded snapshot. Failure: {Failure}", effectiveVersion, githubResult.Message);
+            logger.LogDebug("Aspire skills remote fetch feature '{Feature}' is disabled; using the embedded snapshot.", KnownFeatures.AspireSkillsRemoteFetchEnabled);
         }
 
         var embeddedResult = await InstallFromEmbeddedAsync(cacheRoot, effectiveVersion, activity, cancellationToken).ConfigureAwait(false);
@@ -93,8 +113,8 @@ private async Task<AspireSkillsInstallResult> InstallCoreAsync(CancellationToken
 
         var failureMessage = embeddedResult.Status == AcquisitionStatus.Failed
             ? embeddedResult.Message ?? AgentCommandStrings.AspireSkillsInstaller_GitHubUnavailable
-            : githubResult.Status == AcquisitionStatus.Failed
-                ? githubResult.Message ?? AgentCommandStrings.AspireSkillsInstaller_GitHubUnavailable
+            : githubResult is { Status: AcquisitionStatus.Failed, Message: { } githubMessage }
+                ? githubMessage
                 : AgentCommandStrings.AspireSkillsInstaller_GitHubUnavailable;
 
         activity?.SetStatus(ActivityStatusCode.Error, failureMessage);
@@ -158,14 +178,15 @@ private async Task<AcquisitionResult> InstallFromGitHubAsync(
 
             try
             {
-                var bundle = await CacheArchiveAsync(cacheRoot, archivePath, version, cancellationToken).ConfigureAwait(false);
+                var bundle = await CacheArchiveAsync(cacheRoot, archivePath, version, skipCompatibilityCheck: false, cancellationToken).ConfigureAwait(false);
                 activity?.SetTag("aspire.skills.source", "github");
                 activity?.SetTag("aspire.skills.cache_hit", false);
                 return AcquisitionResult.Installed(bundle);
             }
             catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or InvalidDataException or InvalidOperationException)
             {
-                logger.LogWarning(ex, "Downloaded Aspire skills GitHub release asset {AssetName} is invalid.", asset.Name);
+                // Includes version-mismatch failures from ValidateCompatibility, which fall back to the embedded snapshot.
+                logger.LogDebug(ex, "Downloaded Aspire skills GitHub release asset {AssetName} is invalid.", asset.Name);
                 return AcquisitionResult.Failed(string.Format(CultureInfo.CurrentCulture, AgentCommandStrings.AspireSkillsInstaller_InvalidBundle, ex.Message));
             }
         }
@@ -233,14 +254,20 @@ private async Task<AcquisitionResult> InstallFromEmbeddedAsync(
 
             try
             {
-                var bundle = await CacheArchiveAsync(cacheRoot, archivePath, version, cancellationToken).ConfigureAwait(false);
+                // The embedded snapshot ships inside the CLI binary as the trusted last-resort
+                // fallback. Its `supports` range is stamped at the time the snapshot was built,
+                // which can lag the actual CLI version (especially for prerelease/dogfood builds)
+                // and would otherwise reject a perfectly usable local copy. Skip the bundle's
+                // CLI/SDK compatibility check here so the embedded skills are always offered when
+                // the network path is unavailable.
+                var bundle = await CacheArchiveAsync(cacheRoot, archivePath, version, skipCompatibilityCheck: true, cancellationToken).ConfigureAwait(false);
                 activity?.SetTag("aspire.skills.source", "embedded");
                 activity?.SetTag("aspire.skills.cache_hit", false);
                 return AcquisitionResult.Installed(bundle);
             }
             catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or InvalidDataException or InvalidOperationException)
             {
-                logger.LogWarning(ex, "Embedded Aspire skills bundle {AssetName} is invalid.", metadata.AssetName);
+                logger.LogDebug(ex, "Embedded Aspire skills bundle {AssetName} is invalid.", metadata.AssetName);
                 return AcquisitionResult.Failed(string.Format(CultureInfo.CurrentCulture, AgentCommandStrings.AspireSkillsInstaller_InvalidBundle, ex.Message));
             }
         }
@@ -441,7 +468,12 @@ private static HttpRequestMessage CreateGitHubRequest(string url)
 
         try
         {
-            var bundle = await AspireSkillsBundle.LoadAsync(new DirectoryInfo(cacheDirectory), cancellationToken).ConfigureAwait(false);
+            // Cached bundles are written by this installer (from GitHub or embedded sources).
+            // The cache directory is keyed by bundle version, which is the right invalidation
+            // signal, so skip the `supports` range check here — a previously-embedded snapshot
+            // whose range no longer covers the current CLI is still the local artifact we
+            // chose to use and should not be re-evicted on every invocation.
+            var bundle = await LoadCachedBundleAsync(cacheDirectory, cancellationToken).ConfigureAwait(false);
             ValidateBundleVersion(bundle, version);
             TouchLastUsed(cacheDirectory);
             activity?.SetTag("aspire.skills.cache_hit", true);
@@ -455,10 +487,21 @@ private static HttpRequestMessage CreateGitHubRequest(string url)
         }
     }
 
+    private static Task<AspireSkillsBundle> LoadCachedBundleAsync(string cacheDirectory, CancellationToken cancellationToken)
+    {
+        return AspireSkillsBundle.LoadAsync(
+            new DirectoryInfo(cacheDirectory),
+            VersionHelper.GetDefaultSdkVersion(),
+            VersionHelper.GetDefaultSdkVersion(),
+            skipCompatibilityCheck: true,
+            cancellationToken);
+    }
+
     private async Task<AspireSkillsBundle> CacheArchiveAsync(
         string cacheRoot,
         string archivePath,
         string version,
+        bool skipCompatibilityCheck,
         CancellationToken cancellationToken)
     {
         var extractDir = Path.Combine(cacheRoot, $".extract-{Guid.NewGuid():N}");
@@ -472,7 +515,7 @@ private async Task<AspireSkillsBundle> CacheArchiveAsync(
             var bundleRoot = FindBundleRoot(extractDir);
             CopyDirectory(bundleRoot.FullName, stageDir);
 
-            var stagedBundle = await AspireSkillsBundle.LoadAsync(new DirectoryInfo(stageDir), cancellationToken).ConfigureAwait(false);
+            var stagedBundle = await LoadStagedBundleAsync(stageDir, skipCompatibilityCheck, cancellationToken).ConfigureAwait(false);
             ValidateBundleVersion(stagedBundle, version);
 
             await using var cacheLock = await AcquireCacheLockAsync(cacheRoot, version, cancellationToken).ConfigureAwait(false);
@@ -481,7 +524,7 @@ private async Task<AspireSkillsBundle> CacheArchiveAsync(
             {
                 try
                 {
-                    var existingBundle = await AspireSkillsBundle.LoadAsync(new DirectoryInfo(targetDir), cancellationToken).ConfigureAwait(false);
+                    var existingBundle = await LoadCachedBundleAsync(targetDir, cancellationToken).ConfigureAwait(false);
                     ValidateBundleVersion(existingBundle, version);
                     TouchLastUsed(targetDir);
                     return existingBundle;
@@ -500,7 +543,7 @@ private async Task<AspireSkillsBundle> CacheArchiveAsync(
             Directory.Move(stageDir, targetDir);
             TouchLastUsed(targetDir);
 
-            var installedBundle = await AspireSkillsBundle.LoadAsync(new DirectoryInfo(targetDir), cancellationToken).ConfigureAwait(false);
+            var installedBundle = await LoadCachedBundleAsync(targetDir, cancellationToken).ConfigureAwait(false);
             ValidateBundleVersion(installedBundle, version);
 
             return installedBundle;
@@ -512,6 +555,16 @@ private async Task<AspireSkillsBundle> CacheArchiveAsync(
         }
     }
 
+    private static Task<AspireSkillsBundle> LoadStagedBundleAsync(string stageDir, bool skipCompatibilityCheck, CancellationToken cancellationToken)
+    {
+        return AspireSkillsBundle.LoadAsync(
+            new DirectoryInfo(stageDir),
+            VersionHelper.GetDefaultSdkVersion(),
+            VersionHelper.GetDefaultSdkVersion(),
+            skipCompatibilityCheck,
+            cancellationToken);
+    }
+
     private static async Task<FileStream> AcquireCacheLockAsync(string cacheRoot, string version, CancellationToken cancellationToken)
     {
         var lockPath = Path.Combine(cacheRoot, $".{GetSafeFileName(version)}.lock");