.NET: Improve skill name validation: reject consecutive hyphens and enforce directory name match (#4526)

* improve skill validation * address pr review comments
2026-06-16 21:04:09 +08:00 · 2026-03-06 17:29:53 +00:00
parent 7e98b0cd29
commit 394e9c1692
2 changed files with 47 additions and 8 deletions
@@ -40,9 +40,10 @@ internal sealed partial class FileAgentSkillLoader
    //           "description: \"A skill\"" → (description, A skill, _)
    private static readonly Regex s_yamlKeyValueRegex = new(@"^\s*(\w+)\s*:\s*(?:[""'](.+?)[""']|(.+?))\s*$", RegexOptions.Multiline | RegexOptions.Compiled, TimeSpan.FromSeconds(5));

-    // Validates skill names: lowercase letters, numbers, and hyphens only; must not start or end with a hyphen.
-    // Examples: "my-skill" ✓, "skill123" ✓, "-bad" ✗, "bad-" ✗, "Bad" ✗
-    private static readonly Regex s_validNameRegex = new(@"^[a-z0-9]([a-z0-9\-]*[a-z0-9])?$", RegexOptions.Compiled);
+    // Validates skill names: lowercase letters, numbers, and hyphens only;
+    // must not start or end with a hyphen; must not contain consecutive hyphens.
+    // Examples: "my-skill" ✓, "skill123" ✓, "-bad" ✗, "bad-" ✗, "Bad" ✗, "my--skill" ✗
+    private static readonly Regex s_validNameRegex = new("^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$", RegexOptions.Compiled);

    private readonly ILogger _logger;
    private readonly HashSet<string> _allowedResourceExtensions;
@@ -244,7 +245,22 @@ internal sealed partial class FileAgentSkillLoader

        if (name.Length > MaxNameLength || !s_validNameRegex.IsMatch(name))
        {
-            LogInvalidFieldValue(this._logger, skillFilePath, "name", $"Must be {MaxNameLength} characters or fewer, using only lowercase letters, numbers, and hyphens, and must not start or end with a hyphen.");
+            LogInvalidFieldValue(this._logger, skillFilePath, "name", $"Must be {MaxNameLength} characters or fewer, using only lowercase letters, numbers, and hyphens, and must not start or end with a hyphen or contain consecutive hyphens.");
+            return false;
+        }
+
+        // skillFilePath is e.g. "/skills/my-skill/SKILL.md".
+        // GetDirectoryName strips the filename → "/skills/my-skill".
+        // GetFileName then extracts the last segment → "my-skill".
+        // This gives us the skill's parent directory name to validate against the frontmatter name.
+        string directoryName = Path.GetFileName(Path.GetDirectoryName(skillFilePath)) ?? string.Empty;
+        if (!string.Equals(name, directoryName, StringComparison.Ordinal))
+        {
+            if (this._logger.IsEnabled(LogLevel.Error))
+            {
+                LogNameDirectoryMismatch(this._logger, SanitizePathForLog(skillFilePath), name, SanitizePathForLog(directoryName));
+            }
+
            return false;
        }

@@ -457,6 +473,9 @@ internal sealed partial class FileAgentSkillLoader
    [LoggerMessage(LogLevel.Error, "SKILL.md at '{SkillFilePath}' has an invalid '{FieldName}' value: {Reason}")]
    private static partial void LogInvalidFieldValue(ILogger logger, string skillFilePath, string fieldName, string reason);

+    [LoggerMessage(LogLevel.Error, "SKILL.md at '{SkillFilePath}': skill name '{SkillName}' does not match parent directory name '{DirectoryName}'")]
+    private static partial void LogNameDirectoryMismatch(ILogger logger, string skillFilePath, string skillName, string directoryName);
+
    [LoggerMessage(LogLevel.Warning, "Skipping resource in skill '{SkillName}': '{ResourcePath}' references a path outside the skill directory")]
    private static partial void LogResourcePathTraversal(ILogger logger, string skillName, string resourcePath);

@@ -122,10 +122,11 @@ public sealed class FileAgentSkillLoaderTests : IDisposable
    [InlineData("-leading-hyphen")]
    [InlineData("trailing-hyphen-")]
    [InlineData("has spaces")]
+    [InlineData("consecutive--hyphens")]
    public void DiscoverAndLoadSkills_InvalidName_ExcludesSkill(string invalidName)
    {
        // Arrange
-        string skillDir = Path.Combine(this._testRoot, "invalid-name-test");
+        string skillDir = Path.Combine(this._testRoot, invalidName);
        if (Directory.Exists(skillDir))
        {
            Directory.Delete(skillDir, recursive: true);
@@ -147,15 +148,19 @@ public sealed class FileAgentSkillLoaderTests : IDisposable
    public void DiscoverAndLoadSkills_DuplicateNames_KeepsFirstOnly()
    {
        // Arrange
-        string dir1 = Path.Combine(this._testRoot, "skill-a");
-        string dir2 = Path.Combine(this._testRoot, "skill-b");
+        string dir1 = Path.Combine(this._testRoot, "dupe");
+        string dir2 = Path.Combine(this._testRoot, "subdir");
        Directory.CreateDirectory(dir1);
        Directory.CreateDirectory(dir2);
+
+        // Create a nested duplicate: subdir/dupe/SKILL.md
+        string nestedDir = Path.Combine(dir2, "dupe");
+        Directory.CreateDirectory(nestedDir);
        File.WriteAllText(
            Path.Combine(dir1, "SKILL.md"),
            "---\nname: dupe\ndescription: First\n---\nFirst body.");
        File.WriteAllText(
-            Path.Combine(dir2, "SKILL.md"),
+            Path.Combine(nestedDir, "SKILL.md"),
            "---\nname: dupe\ndescription: Second\n---\nSecond body.");

        // Act
@@ -168,6 +173,21 @@ public sealed class FileAgentSkillLoaderTests : IDisposable
        Assert.True(desc == "First" || desc == "Second", $"Unexpected description: {desc}");
    }

+    [Fact]
+    public void DiscoverAndLoadSkills_NameMismatchesDirectory_ExcludesSkill()
+    {
+        // Arrange — directory name differs from the frontmatter name
+        _ = this.CreateSkillDirectoryWithRawContent(
+            "wrong-dir-name",
+            "---\nname: actual-skill-name\ndescription: A skill\n---\nBody.");
+
+        // Act
+        var skills = this._loader.DiscoverAndLoadSkills(new[] { this._testRoot });
+
+        // Assert
+        Assert.Empty(skills);
+    }
+
    [Fact]
    public void DiscoverAndLoadSkills_FilesWithMatchingExtensions_DiscoveredAsResources()
    {