commit d650436ce907357cbd8e079cec603c40f794dd3b
Author: Ben Sima <ben@bensima.com>
Date: Thu Jan 1 11:30:36 2026
Add skill validation with descriptive errors (t-305)
Changed parseSkillMd to return Either SkillValidationError instead of Maybe:
- MissingFrontmatter: SKILL.md doesn't start with '---'
- MalformedFrontmatter: Missing closing '---' or YAML parse error
- MissingRequiredField: Missing 'name' or 'description'
- EmptyBody: No content after frontmatter
Added:
- SkillValidationError type with formatValidationError helper
- validateSkill function to check without loading
- validateSkillTool for agents to check skills
- Unit tests for each validation error type
Task-Id: t-305
diff --git a/Omni/Agent/Skills.hs b/Omni/Agent/Skills.hs
index 64b5ff80..522eef11 100644
--- a/Omni/Agent/Skills.hs
+++ b/Omni/Agent/Skills.hs
@@ -21,14 +21,18 @@
module Omni.Agent.Skills
( Skill (..),
SkillMetadata (..),
+ SkillValidationError (..),
loadSkill,
loadSkillMetadata,
listSkills,
listSkillsForUser,
publishSkill,
+ validateSkill,
+ parseSkillMd,
skillTool,
listSkillsTool,
publishSkillTool,
+ validateSkillTool,
skillsDir,
main,
test,
@@ -60,8 +64,8 @@ test =
Test.unit "SkillMetadata parses from YAML frontmatter" <| do
let yaml = "name: test-skill\ndescription: A test skill"
case parseYamlFrontmatter yaml of
- Nothing -> Test.assertFailure "Failed to parse frontmatter"
- Just meta -> do
+ Left err -> Test.assertFailure ("Failed to parse frontmatter: " <> Text.unpack err)
+ Right meta -> do
skillMetaName meta Test.@=? "test-skill"
skillMetaDescription meta Test.@=? "A test skill",
Test.unit "parseSkillMd extracts frontmatter and body" <| do
@@ -74,8 +78,8 @@ test =
\\n\
\Instructions here."
case parseSkillMd content of
- Nothing -> Test.assertFailure "Failed to parse SKILL.md"
- Just (meta, body) -> do
+ Left err -> Test.assertFailure ("Failed to parse SKILL.md: " <> Text.unpack (formatValidationError err))
+ Right (meta, body) -> do
skillMetaName meta Test.@=? "my-skill"
("# My Skill" `Text.isInfixOf` body) Test.@=? True,
Test.unit "skillTool schema is valid" <| do
@@ -99,7 +103,32 @@ test =
Test.unit "capitalizeFirst works" <| do
capitalizeFirst "product" Test.@=? "Product"
capitalizeFirst "" Test.@=? ""
- capitalizeFirst "ALREADY" Test.@=? "ALREADY"
+ capitalizeFirst "ALREADY" Test.@=? "ALREADY",
+ -- Validation error tests
+ Test.unit "parseSkillMd detects missing frontmatter" <| do
+ let content = "# No frontmatter here\n\nJust body text."
+ case parseSkillMd content of
+ Left MissingFrontmatter -> pure ()
+ Left other -> Test.assertFailure ("Expected MissingFrontmatter, got: " <> show other)
+ Right _ -> Test.assertFailure "Should have failed with MissingFrontmatter",
+ Test.unit "parseSkillMd detects unclosed frontmatter" <| do
+ let content = "---\nname: test\ndescription: test\nno closing delimiter"
+ case parseSkillMd content of
+ Left (MalformedFrontmatter _) -> pure ()
+ Left other -> Test.assertFailure ("Expected MalformedFrontmatter, got: " <> show other)
+ Right _ -> Test.assertFailure "Should have failed with MalformedFrontmatter",
+ Test.unit "parseSkillMd detects empty body" <| do
+ let content = "---\nname: test\ndescription: test\n---\n \n"
+ case parseSkillMd content of
+ Left EmptyBody -> pure ()
+ Left other -> Test.assertFailure ("Expected EmptyBody, got: " <> show other)
+ Right _ -> Test.assertFailure "Should have failed with EmptyBody",
+ Test.unit "parseSkillMd detects missing name field" <| do
+ let content = "---\ndescription: test\n---\nBody here"
+ case parseSkillMd content of
+ Left (MalformedFrontmatter _) -> pure () -- Missing name in YAML
+ Left other -> Test.assertFailure ("Expected MalformedFrontmatter, got: " <> show other)
+ Right _ -> Test.assertFailure "Should have failed with missing name"
]
-- | Base directory for all skills
@@ -126,16 +155,37 @@ instance Aeson.ToJSON SkillMetadata where
"description" .= skillMetaDescription m
]
+-- | Skill validation errors
+data SkillValidationError
+ = MissingFrontmatter
+ | MalformedFrontmatter Text
+ | MissingRequiredField Text -- "name" or "description"
+ | EmptyBody
+ deriving (Show, Eq, Generic)
+
+-- | Format validation error as human-readable message
+formatValidationError :: SkillValidationError -> Text
+formatValidationError MissingFrontmatter =
+ "Missing YAML frontmatter. SKILL.md must start with '---'"
+formatValidationError (MalformedFrontmatter reason) =
+ "Malformed YAML frontmatter: " <> reason
+formatValidationError (MissingRequiredField field) =
+ "Missing required field: " <> field
+formatValidationError EmptyBody =
+ "Empty skill body. Add instructions after the frontmatter."
+
-- | Simple YAML frontmatter parser for skill metadata
-- Parses lines like "name: value" and "description: value"
-parseYamlFrontmatter :: Text -> Maybe SkillMetadata
+parseYamlFrontmatter :: Text -> Either Text SkillMetadata
parseYamlFrontmatter yaml = do
let kvPairs = parseKvLines (Text.lines yaml)
getName = List.lookup "name" kvPairs
getDesc = List.lookup "description" kvPairs
- name' <- getName
- desc <- getDesc
- pure SkillMetadata {skillMetaName = name', skillMetaDescription = desc}
+ case getName of
+ Nothing -> Left "Missing 'name' field"
+ Just name' -> case getDesc of
+ Nothing -> Left "Missing 'description' field"
+ Just desc -> Right SkillMetadata {skillMetaName = name', skillMetaDescription = desc}
where
parseKvLines :: [Text] -> [(Text, Text)]
parseKvLines = mapMaybe parseKvLine
@@ -245,17 +295,38 @@ injectRoles body = do
then pure <| Left (Text.intercalate "; " errors)
else pure <| Right <| formatRoleDefinitions loaded <> body
--- | Parse SKILL.md content into metadata and body
-parseSkillMd :: Text -> Maybe (SkillMetadata, Text)
+-- | Parse SKILL.md content into metadata and body with validation
+parseSkillMd :: Text -> Either SkillValidationError (SkillMetadata, Text)
parseSkillMd content = do
let stripped = Text.strip content
- guard (Text.isPrefixOf "---" stripped)
+
+ -- Check for opening ---
+ unless (Text.isPrefixOf "---" stripped) <|
+ Left MissingFrontmatter
+
let afterFirst = Text.drop 3 stripped
(yamlPart, rest) = Text.breakOn "---" (Text.stripStart afterFirst)
- guard (not (Text.null rest))
+
+ -- Check for closing ---
+ when (Text.null rest) <|
+ Left (MalformedFrontmatter "Missing closing '---'")
+
let body = Text.strip (Text.drop 3 rest)
- meta <- parseYamlFrontmatter (Text.strip yamlPart)
- pure (meta, body)
+
+ -- Check for non-empty body
+ when (Text.null body) <|
+ Left EmptyBody
+
+ -- Parse frontmatter
+ case parseYamlFrontmatter (Text.strip yamlPart) of
+ Left err -> Left (MalformedFrontmatter err)
+ Right meta -> do
+ -- Validate required fields
+ when (Text.null (skillMetaName meta)) <|
+ Left (MissingRequiredField "name")
+ when (Text.null (skillMetaDescription meta)) <|
+ Left (MissingRequiredField "description")
+ Right (meta, body)
-- | Load just the metadata for a skill (for progressive disclosure)
loadSkillMetadata :: FilePath -> IO (Maybe SkillMetadata)
@@ -265,7 +336,9 @@ loadSkillMetadata skillDir = do
if exists
then do
content <- TextIO.readFile skillMd
- pure (fst </ parseSkillMd content)
+ pure <| case parseSkillMd content of
+ Right (meta, _) -> Just meta
+ Left _ -> Nothing
else pure Nothing
-- | Load a full skill by name for a user
@@ -299,8 +372,8 @@ loadSkill userName skillName' = do
then do
content <- TextIO.readFile skillMd
case parseSkillMd content of
- Nothing -> pure <| Left "Failed to parse SKILL.md frontmatter"
- Just (meta, body) -> do
+ Left err -> pure <| Left (formatValidationError err)
+ Right (meta, body) -> do
-- Inject role definitions if skill has <role:X> tags
enrichedBodyResult <- injectRoles body
case enrichedBodyResult of
@@ -511,3 +584,65 @@ executePublishSkill userName v =
case result of
Left err -> pure <| mkError err
Right msg -> pure <| mkSuccess msg
+
+-- | Validate a skill without loading it
+validateSkill :: Text -> Text -> IO (Either Text ())
+validateSkill userName skillName' = do
+ let userDir = skillsDir FilePath.</> Text.unpack userName FilePath.</> Text.unpack skillName'
+ sharedDir = skillsDir FilePath.</> "shared" FilePath.</> Text.unpack skillName'
+
+ userExists <- Directory.doesDirectoryExist userDir
+ sharedExists <- Directory.doesDirectoryExist sharedDir
+
+ let targetDir
+ | userExists = Just userDir
+ | sharedExists = Just sharedDir
+ | otherwise = Nothing
+
+ case targetDir of
+ Nothing -> pure <| Left ("Skill not found: " <> skillName')
+ Just dir -> do
+ let skillMd = dir FilePath.</> "SKILL.md"
+ exists <- Directory.doesFileExist skillMd
+ if exists
+ then do
+ content <- TextIO.readFile skillMd
+ case parseSkillMd content of
+ Left err -> pure <| Left (formatValidationError err)
+ Right _ -> pure <| Right ()
+ else pure <| Left ("SKILL.md not found in " <> Text.pack dir)
+
+-- | Tool to validate a skill's structure
+validateSkillTool :: Text -> Engine.Tool
+validateSkillTool userName =
+ Engine.Tool
+ { Engine.toolName = "validate_skill",
+ Engine.toolDescription =
+ "Validate a skill's SKILL.md syntax and structure without loading it. "
+ <> "Checks for: proper frontmatter, required name and description fields, "
+ <> "and non-empty body.",
+ Engine.toolJsonSchema =
+ Aeson.object
+ [ "type" .= ("object" :: Text),
+ "properties"
+ .= Aeson.object
+ [ "name"
+ .= Aeson.object
+ [ "type" .= ("string" :: Text),
+ "description" .= ("Name of the skill to validate" :: Text)
+ ]
+ ],
+ "required" .= (["name"] :: [Text])
+ ],
+ Engine.toolExecute = executeValidateSkill userName
+ }
+
+executeValidateSkill :: Text -> Aeson.Value -> IO Aeson.Value
+executeValidateSkill userName v =
+ case Aeson.fromJSON v of
+ Aeson.Error e -> pure <| mkError (Text.pack e)
+ Aeson.Success args -> do
+ result <- validateSkill userName (skillArgsName args)
+ case result of
+ Left err -> pure <| mkError err
+ Right () -> pure <| mkSuccess ("Skill '" <> skillArgsName args <> "' is valid")