From 240c75229edf1157fb7bacdbcd555996b33ab6ef Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 12 Mar 2026 10:01:10 +0530 Subject: [PATCH 01/16] Support Agent Skills - Take I --- ballerina-interpreter/Dependencies.toml | 10 +- ballerina-interpreter/README.md | 10 + ballerina-interpreter/agent.bal | 40 +++- ballerina-interpreter/main.bal | 9 +- ballerina-interpreter/parser.bal | 10 +- ballerina-interpreter/skills.bal | 265 +++++++++++++++++++++ ballerina-interpreter/tests/agent_test.bal | 4 +- ballerina-interpreter/types.bal | 16 ++ templates/agent_template.afm.md | 7 + 9 files changed, 348 insertions(+), 23 deletions(-) create mode 100644 ballerina-interpreter/skills.bal diff --git a/ballerina-interpreter/Dependencies.toml b/ballerina-interpreter/Dependencies.toml index 9850b6f..37caeb7 100644 --- a/ballerina-interpreter/Dependencies.toml +++ b/ballerina-interpreter/Dependencies.toml @@ -5,7 +5,7 @@ [ballerina] dependencies-toml-version = "2" -distribution-version = "2201.12.7" +distribution-version = "2201.12.10" [[package]] org = "ballerina" @@ -90,7 +90,7 @@ dependencies = [ [[package]] org = "ballerina" name = "data.xmldata" -version = "1.5.2" +version = "1.6.1" dependencies = [ {org = "ballerina", name = "jballerina.java"}, {org = "ballerina", name = "lang.object"} @@ -117,6 +117,9 @@ dependencies = [ {org = "ballerina", name = "os"}, {org = "ballerina", name = "time"} ] +modules = [ + {org = "ballerina", packageName = "file", moduleName = "file"} +] [[package]] org = "ballerina" @@ -468,7 +471,7 @@ modules = [ [[package]] org = "ballerinax" name = "ai.anthropic" -version = "1.3.0" +version = "1.3.1" dependencies = [ {org = "ballerina", name = "ai"}, {org = "ballerina", name = "constraint"}, @@ -530,6 +533,7 @@ version = "0.1.0" dependencies = [ {org = "ballerina", name = "ai"}, {org = "ballerina", name = "data.yaml"}, + {org = "ballerina", name = "file"}, {org = "ballerina", name = "http"}, {org = "ballerina", name = "io"}, {org = "ballerina", name = "jballerina.java"}, diff --git a/ballerina-interpreter/README.md b/ballerina-interpreter/README.md index c76ca18..1937029 100644 --- a/ballerina-interpreter/README.md +++ b/ballerina-interpreter/README.md @@ -48,8 +48,17 @@ docker run -v /path/to/agent.afm.md:/app/agent.afm.md \ -e afmFilePath=/app/agent.afm.md \ -p 8085:8085 \ afm-ballerina-interpreter + +# Run with skills (mount the skills directory so the agent can discover them) +docker run -v /path/to/agent.afm.md:/app/agent.afm.md \ + -v /path/to/skills:/app/skills \ + -e afmFilePath=/app/agent.afm.md \ + -p 8085:8085 \ + afm-ballerina-interpreter ``` +When using local skills, the `path` in the AFM file should be relative to the AFM file's location. For example, if the AFM file is at `/app/agent.afm.md` and skills are mounted at `/app/skills`, use `path: "./skills"` in the AFM file. + ## Testing ```bash @@ -68,6 +77,7 @@ ballerina-interpreter/ ├── interface_web_chat.bal # Web chat HTTP API ├── interface_web_ui.bal # Web chat UI ├── interface_webhook.bal # Webhook/WebSub handler +├── skills.bal # Agent Skills discovery & toolkit ├── modules/ │ └── everit.validator/ # JSON Schema validation ├── tests/ # Test files diff --git a/ballerina-interpreter/agent.bal b/ballerina-interpreter/agent.bal index bad401d..61ffb38 100644 --- a/ballerina-interpreter/agent.bal +++ b/ballerina-interpreter/agent.bal @@ -23,10 +23,10 @@ import ballerina/http; import ballerinax/ai.anthropic; import ballerinax/ai.openai; -function createAgent(AFMRecord afmRecord) returns ai:Agent|error { +function createAgent(AFMRecord afmRecord, string afmFileDir) returns ai:Agent|error { AFMRecord {metadata, role, instructions} = afmRecord; - ai:McpToolKit[] mcpToolkits = []; + ai:McpToolKit[] mcpToolKits = []; MCPServer[]? mcpServers = metadata?.tools?.mcp; if mcpServers is MCPServer[] { foreach MCPServer mcpConn in mcpServers { @@ -36,7 +36,7 @@ function createAgent(AFMRecord afmRecord) returns ai:Agent|error { } string[]? filteredTools = getFilteredTools(mcpConn.tool_filter); - mcpToolkits.push(check new ai:McpToolKit( + mcpToolKits.push(check new ai:McpToolKit( transport.url, permittedTools = filteredTools, auth = check mapToHttpClientAuth(transport.authentication) @@ -44,14 +44,27 @@ function createAgent(AFMRecord afmRecord) returns ai:Agent|error { } } + [string, SkillsToolKit]? catalog = check extractSkillCatalog(metadata, afmFileDir); + + string effectiveInstructions; + (ai:BaseToolKit)[] toolKits; + + if catalog is () { + effectiveInstructions = instructions; + toolKits = mcpToolKits; + } else { + effectiveInstructions = string `${instructions}\n${catalog[0]}`; + toolKits = [...mcpToolKits, catalog[1]]; + } + ai:ModelProvider model = check getModel(metadata?.model); - + ai:AgentConfiguration agentConfig = { systemPrompt: { - role, - instructions + role, + instructions: effectiveInstructions }, - tools: mcpToolkits, + tools: toolKits, model }; @@ -85,9 +98,9 @@ function getModel(Model? model) returns ai:ModelProvider|error { return error("This implementation requires the 'provider' of the model to be specified"); } - provider = provider.toLowerAscii(); + string providerLower = provider.toLowerAscii(); - if provider == "wso2" { + if providerLower == "wso2" { return new ai:Wso2ModelProvider( model.url ?: "https://dev-tools.wso2.com/ballerina-copilot/v2.0", check getToken(model.authentication) @@ -99,7 +112,7 @@ function getModel(Model? model) returns ai:ModelProvider|error { return error("This implementation requires the 'name' of the model to be specified"); } - match provider { + match providerLower { "openai" => { return new openai:ModelProvider( check getApiKey(model.authentication), @@ -115,12 +128,13 @@ function getModel(Model? model) returns ai:ModelProvider|error { ); } } - return error(string `Model provider: ${provider} not yet supported`); + return error(string `Model provider: ${provider} not yet supported`); } const DEFAULT_SESSION_ID = "sessionId"; -function runAgent(ai:Agent agent, json payload, map? inputSchema = (), map? outputSchema = (), string sessionId = DEFAULT_SESSION_ID) +function runAgent(ai:Agent agent, json payload, map? inputSchema = (), + map? outputSchema = (), string sessionId = DEFAULT_SESSION_ID) returns json|InputError|AgentError { error? validateJsonSchemaResult = validateJsonSchema(inputSchema, payload); if validateJsonSchemaResult is error { @@ -275,7 +289,7 @@ isolated function validateJsonSchema(map? jsonSchemaVal, json sampleJson) validator:JSONObject jsonObject = validator:newJSONObject7(sampleJson.toJsonString()); error? validationResult = trap schema.validate(jsonObject); if validationResult is error { - return error("JSON validation failed: " + validationResult.message()); + return error(string `JSON validation failed: ${validationResult.message()}`); } return (); } diff --git a/ballerina-interpreter/main.bal b/ballerina-interpreter/main.bal index 1cf8b0e..ebadc8f 100644 --- a/ballerina-interpreter/main.bal +++ b/ballerina-interpreter/main.bal @@ -15,6 +15,7 @@ // under the License. import ballerina/ai; +import ballerina/file; import ballerina/http; import ballerina/io; import ballerina/log; @@ -41,14 +42,14 @@ public function main(string? filePath = ()) returns error? { fileToUse = filePath; } - string content = check io:fileReadString(fileToUse); + string afmFileDir = check file:parentPath(check file:getAbsolutePath(fileToUse)); AFMRecord afm = check parseAfm(content); - check runAgentFromAFM(afm, port); + check runAgentFromAFM(afm, port, afmFileDir); } -function runAgentFromAFM(AFMRecord afm, int port) returns error? { +function runAgentFromAFM(AFMRecord afm, int port, string afmFileDir) returns error? { AgentMetadata metadata = afm.metadata; Interface[] agentInterfaces = metadata.interfaces ?: [{}]; @@ -56,7 +57,7 @@ function runAgentFromAFM(AFMRecord afm, int port) returns error? { var [consoleChatInterface, webChatInterface, webhookInterface] = check validateAndExtractInterfaces(agentInterfaces); - ai:Agent agent = check createAgent(afm); + ai:Agent agent = check createAgent(afm, afmFileDir); // Start all service-based interfaces first (non-blocking) http:Listener? httpListener = (); diff --git a/ballerina-interpreter/parser.bal b/ballerina-interpreter/parser.bal index 3420f39..c046604 100644 --- a/ballerina-interpreter/parser.bal +++ b/ballerina-interpreter/parser.bal @@ -161,7 +161,7 @@ function validateHttpVariables(AFMRecord afmRecord) returns error? { return error("http: variables are only supported in webhook prompt fields, found in instructions section"); } - AgentMetadata {authors, provider, model, interfaces, tools, max_iterations: _, ...rest} = afmRecord.metadata; + AgentMetadata {authors, provider, model, interfaces, tools, skills, max_iterations: _, ...rest} = afmRecord.metadata; string[] erroredKeys = []; @@ -285,6 +285,14 @@ function validateHttpVariables(AFMRecord afmRecord) returns error? { } } + if skills is SkillSource[] { + foreach SkillSource skillSource in skills { + if containsHttpVariable(skillSource.path) { + erroredKeys.push("skills.path"); + } + } + } + if erroredKeys.length() > 0 { return error(string `http: variables are only supported in webhook prompt fields, found in metadata fields: ${string:'join(", ", ...erroredKeys)}`); } diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal new file mode 100644 index 0000000..3fd6971 --- /dev/null +++ b/ballerina-interpreter/skills.bal @@ -0,0 +1,265 @@ +// Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +// +// WSO2 LLC. licenses this file to you under the Apache License, +// Version 2.0 (the "License"); you may not use this file except +// in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +import ballerina/ai; +import ballerina/data.yaml; +import ballerina/file; +import ballerina/io; +import ballerina/log; + +const SKILL_FILE = "SKILL.md"; +const REFERENCES_DIR = "references"; +const ASSETS_DIR = "assets"; + +function extractSkillCatalog(AgentMetadata metadata, string afmFileDir) returns [string, SkillsToolKit]|error? { + SkillSource[]? skillSources = metadata?.skills; + if skillSources is () || skillSources.length() == 0 { + return (); + } + map skills = check discoverSkills(skillSources, afmFileDir); + log:printDebug(string `Loaded ${skills.length()} skill(s): ${string:'join(", ", ...skills.keys())}`); + string? skillCatalog = buildSkillCatalog(skills); + if skillCatalog is () { + return (); // No catalog if no skills, even if skill sources were defined + } + return [skillCatalog, new SkillsToolKit(skills)]; +} + +type SkillFrontmatter record { + string name; + string description; +}; + +function discoverSkills(SkillSource[] sources, string afmFileDir) returns map|error { + map skills = {}; + + foreach SkillSource 'source in sources { + string resolvedPath = check file:joinPath(afmFileDir, 'source.path); + map localSkills = check discoverLocalSkills(resolvedPath); + foreach [string, SkillInfo] [name, info] in localSkills.entries() { + if skills.hasKey(name) { + log:printWarn(string `Skill '${name}' already discovered, skipping duplicate from ${'source.path}`); + continue; + } + skills[name] = info; + } + } + + return skills; +} + +function discoverLocalSkills(string path) returns map|error { + map skills = {}; + string absPath = check file:getAbsolutePath(path); + + string skillMdPath = check file:joinPath(absPath, SKILL_FILE); + if check file:test(skillMdPath, file:EXISTS) { + SkillInfo info = check parseSkillMd(skillMdPath, absPath); + return {[info.name]: info}; + } + + file:MetaData[] entries = check file:readDir(absPath); + foreach file:MetaData entry in entries { + if !entry.dir { + continue; + } + string subSkillMd = check file:joinPath(entry.absPath, SKILL_FILE); + if !check file:test(subSkillMd, file:EXISTS) { + continue; + } + + SkillInfo|error info = parseSkillMd(subSkillMd, entry.absPath); + if info is error { + log:printError(string `Failed to parse skill at ${entry.absPath}`, 'error = info); + continue; + } + if skills.hasKey(info.name) { + log:printWarn(string `Skill '${info.name}' already discovered, skipping duplicate at ${entry.absPath}`); + continue; + } + skills[info.name] = info; + } + + return skills; +} + +function parseSkillMd(string skillMdPath, string basePath) returns SkillInfo|error { + string content = check io:fileReadString(skillMdPath); + return parseSkillMdContent(content, basePath, listLocalResources(basePath)); +} + +function parseSkillMdContent(string content, string basePath, string[] resources) returns SkillInfo|error { + string[] lines = splitLines(content); + int length = lines.length(); + + if length == 0 || lines[0].trim() != FRONTMATTER_DELIMITER { + return error("SKILL.md must start with YAML frontmatter (---)"); + } + + int i = 1; + while i < length && lines[i].trim() != FRONTMATTER_DELIMITER { + i += 1; + } + + if i >= length { + return error("SKILL.md frontmatter is not closed (missing ---)"); + } + + string yamlContent = string:'join("\n", ...lines.slice(1, i)); + map intermediate = check yaml:parseString(yamlContent); + SkillFrontmatter frontmatter = check intermediate.fromJsonWithType(); + + if frontmatter.name.trim() == "" { + return error("SKILL.md 'name' field is required and must not be empty"); + } + if frontmatter.description.trim() == "" { + return error("SKILL.md 'description' field is required and must not be empty"); + } + + string body = string:'join("\n", ...lines.slice(i + 1)).trim(); + + return { + name: frontmatter.name, + description: frontmatter.description, + body, + basePath, + resources + }; +} + +function listLocalResources(string basePath) returns string[] { + string[] resources = []; + foreach string dir in [REFERENCES_DIR, ASSETS_DIR] { + do { + string dirPath = check file:joinPath(basePath, dir); + if !check file:test(dirPath, file:EXISTS) { + continue; + } + file:MetaData[] entries = check file:readDir(dirPath); + foreach file:MetaData entry in entries { + if !entry.dir { + resources.push(check file:joinPath(dir, getFileName(entry.absPath))); + } + } + } on fail error e { + log:printWarn(string `Failed to read directory ${dir}`, 'error = e); + } + } + return resources; +} + +function getFileName(string path) returns string { + int? lastSlash = path.lastIndexOf("/") ?: path.lastIndexOf("\\"); + return lastSlash is int ? path.substring(lastSlash + 1) : path; +} + +function buildSkillCatalog(map skills) returns string? { + if skills.length() == 0 { + return (); + } + + return string ` +## Available Skills + +The following skills provide specialized instructions for specific tasks. +When a task matches a skill's description, call the activate_skill tool +with the skill's name to load its full instructions. + +${xml ` +${from SkillInfo skill in skills select xml ` + + ${skill.name} + ${skill.description} + +`} +`.toString()} +`; +} + +isolated class SkillsToolKit { + *ai:BaseToolKit; + + private final map & readonly skills; + + isolated function init(map skills) { + self.skills = skills.cloneReadOnly(); + } + + # Activates a skill by name and returns its full instructions along with available resources. + # Call this when a task matches one of the available skills' descriptions. + # + # + name - the name of the skill to activate (must match a name from the available skills catalog) + # + return - the skill's full instructions and list of available resource files + @ai:AgentTool + isolated function activate_skill(string name) returns string|error { + lock { + if !self.skills.hasKey(name) { + return error(string `Skill '${name}' not found. Available skills: ${ + string:'join(", ", ...self.skills.keys())}`); + } + + SkillInfo info = self.skills.get(name); + return xml ` + +${info.body} +${info.resources.length() > 0 ? + xml ` + +${from string res in info.resources select xml `${res}`} + +Use the read_skill_resource tool to read any of these files if needed. +` : xml ``} + +`.toString(); + } + } + + # Reads a resource file from a skill's references/ or assets/ directory. + # Only files listed in skill_resources after activating a skill can be read. + # + # + skillName - the name of the skill that owns the resource + # + resourcePath - relative path to the resource file (e.g., "references/REFERENCE.md" or "assets/template.json") + # + return - the content of the resource file + @ai:AgentTool + isolated function read_skill_resource(string skillName, string resourcePath) returns string|error { + lock { + if !self.skills.hasKey(skillName) { + return error(string `Skill '${skillName}' not found`); + } + + if !resourcePath.startsWith(string `${REFERENCES_DIR}/`) && + !resourcePath.startsWith(string `${ASSETS_DIR}/`) { + return error(string `Resource path must start with '${REFERENCES_DIR}/' or '${ASSETS_DIR}/'`); + } + + if resourcePath.indexOf("../") != () || resourcePath.indexOf("..\\") != () { + return error("Path traversal is not allowed in resource paths"); + } + + SkillInfo info = self.skills.get(skillName); + if info.resources.indexOf(resourcePath) is () { + return error(string `Resource '${resourcePath}' not found in skill '${ + skillName}'. Available: ${string:'join(", ", ...info.resources)}`); + } + + string fullPath = check file:joinPath(info.basePath, resourcePath); + return io:fileReadString(fullPath); + } + } + + public isolated function getTools() returns ai:ToolConfig[] => + ai:getToolConfigs([self.activate_skill, self.read_skill_resource]); +} diff --git a/ballerina-interpreter/tests/agent_test.bal b/ballerina-interpreter/tests/agent_test.bal index f1cadd1..c117be2 100644 --- a/ballerina-interpreter/tests/agent_test.bal +++ b/ballerina-interpreter/tests/agent_test.bal @@ -369,7 +369,7 @@ function testArrayOutputSchemaEndToEnd() returns error? { AFMRecord afm = check parseAfm(content); int testPort = 9195; - future _ = start runAgentFromAFM(afm, testPort); + future _ = start runAgentFromAFM(afm, testPort, "tests"); // Wait for agent to start runtime:sleep(2); @@ -396,7 +396,7 @@ function testArrayOutputSchemaInvalidResponse() returns error? { AFMRecord afm = check parseAfm(content); int testPort = 9196; - future _ = start runAgentFromAFM(afm, testPort); + future _ = start runAgentFromAFM(afm, testPort, "tests"); // Wait for agent to start runtime:sleep(2); diff --git a/ballerina-interpreter/types.bal b/ballerina-interpreter/types.bal index 0ff5079..77ced96 100644 --- a/ballerina-interpreter/types.bal +++ b/ballerina-interpreter/types.bal @@ -65,6 +65,21 @@ type Tools record {| MCPServer[] mcp?; |}; +type LocalSkillSource record {| + "local" 'type = "local"; + string path; +|}; + +type SkillSource LocalSkillSource; + +type SkillInfo record {| + string name; + string description; + string body; + string basePath; + string[] resources; +|}; + type Parameter record {| string name; string 'type; @@ -154,6 +169,7 @@ type AgentMetadata record {| Model model?; Interface[] interfaces?; Tools tools?; + SkillSource[] skills?; int max_iterations?; |}; diff --git a/templates/agent_template.afm.md b/templates/agent_template.afm.md index 515a9a1..b31b8ce 100644 --- a/templates/agent_template.afm.md +++ b/templates/agent_template.afm.md @@ -155,6 +155,13 @@ tools: allow: - "query" - "search" +# ============================================================================ +# SKILLS - OPTIONAL (Agent Skills format: https://agentskills.io) +# ============================================================================ +skills: + # Local skills directory or (a directory that may contain multiple skill subdirectories) + - type: "local" + path: "./skills" --- # Role From c5ab98ec3e9b2e554fb1023c578a5a864d3a802f Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 12 Mar 2026 17:40:35 +0530 Subject: [PATCH 02/16] Fix up code and add tests --- ballerina-interpreter/agent.bal | 2 +- ballerina-interpreter/parser.bal | 79 +-- ballerina-interpreter/skills.bal | 105 ++- .../duplicate_names/error_translator/SKILL.md | 8 + .../error_translator_dup/SKILL.md | 8 + .../tests/skills/empty_dir/.gitkeep | 0 .../tests/skills/invalid_skill/SKILL.md | 6 + .../skills/multi_skills/pr_summary/SKILL.md | 12 + .../multi_skills/security_review/SKILL.md | 15 + .../security_review/assets/template.json | 1 + .../security_review/references/REFERENCE.md | 11 + .../tests/skills/no_frontmatter/SKILL.md | 2 + .../partial_invalid/invalid_skill/SKILL.md | 6 + .../partial_invalid/valid_skill/SKILL.md | 6 + .../tests/skills/single_skill/SKILL.md | 12 + .../skills/source1/release_notes/SKILL.md | 8 + .../skills/source2/release_notes/SKILL.md | 8 + ballerina-interpreter/tests/skills_test.bal | 599 ++++++++++++++++++ 18 files changed, 782 insertions(+), 106 deletions(-) create mode 100644 ballerina-interpreter/tests/skills/duplicate_names/error_translator/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/empty_dir/.gitkeep create mode 100644 ballerina-interpreter/tests/skills/invalid_skill/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/multi_skills/pr_summary/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/multi_skills/security_review/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/multi_skills/security_review/assets/template.json create mode 100644 ballerina-interpreter/tests/skills/multi_skills/security_review/references/REFERENCE.md create mode 100644 ballerina-interpreter/tests/skills/no_frontmatter/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/partial_invalid/invalid_skill/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/partial_invalid/valid_skill/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/single_skill/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/source1/release_notes/SKILL.md create mode 100644 ballerina-interpreter/tests/skills/source2/release_notes/SKILL.md create mode 100644 ballerina-interpreter/tests/skills_test.bal diff --git a/ballerina-interpreter/agent.bal b/ballerina-interpreter/agent.bal index 61ffb38..e6f8a55 100644 --- a/ballerina-interpreter/agent.bal +++ b/ballerina-interpreter/agent.bal @@ -53,7 +53,7 @@ function createAgent(AFMRecord afmRecord, string afmFileDir) returns ai:Agent|er effectiveInstructions = instructions; toolKits = mcpToolKits; } else { - effectiveInstructions = string `${instructions}\n${catalog[0]}`; + effectiveInstructions = string `${instructions}\n\n${catalog[0]}`; toolKits = [...mcpToolKits, catalog[1]]; } diff --git a/ballerina-interpreter/parser.bal b/ballerina-interpreter/parser.bal index c046604..d12296e 100644 --- a/ballerina-interpreter/parser.bal +++ b/ballerina-interpreter/parser.bal @@ -19,40 +19,19 @@ import ballerina/os; function parseAfm(string content) returns AFMRecord|error { string resolvedContent = check resolveVariables(content); - - string[] lines = splitLines(resolvedContent); - int length = lines.length(); - - AgentMetadata? metadata = (); - int bodyStart = 0; - - // Extract and parse YAML frontmatter - if length > 0 && lines[0].trim() == FRONTMATTER_DELIMITER { - int i = 1; - while i < length && lines[i].trim() != FRONTMATTER_DELIMITER { - i += 1; - } - - if i < length { - string[] fmLines = []; - foreach int j in 1 ..< i { - fmLines.push(lines[j]); - } - string yamlContent = string:'join("\n", ...fmLines); - map intermediate = check yaml:parseString(yamlContent); - metadata = check intermediate.fromJsonWithType(); - bodyStart = i + 1; - } - } - + + [map, string] [frontmatterMap, body] = check extractFrontmatter(resolvedContent); + AgentMetadata metadata = check frontmatterMap.fromJsonWithType(); + // Extract Role and Instructions sections + string[] bodyLines = splitLines(body); string role = ""; string instructions = ""; boolean inRole = false; boolean inInstructions = false; - - foreach int k in bodyStart ..< length { - string line = lines[k]; + + foreach int k in 0 ..< bodyLines.length() { + string line = bodyLines[k]; string trimmed = line.trim(); if trimmed.startsWith("# ") { @@ -70,7 +49,7 @@ function parseAfm(string content) returns AFMRecord|error { } AFMRecord afmRecord = { - metadata: check metadata.ensureType(), + metadata, role: role.trim(), instructions: instructions.trim() }; @@ -406,19 +385,43 @@ function toolFilterContainsHttpVariable(ToolFilter? filter) returns boolean { return false; } +// Extracts YAML frontmatter and the remaining body from a document delimited by `---`. +// Returns the parsed YAML as a map and the body text after the closing delimiter. +function extractFrontmatter(string content) returns [map, string]|error { + string[] lines = splitLines(content); + int length = lines.length(); + + if length == 0 || lines[0].trim() != FRONTMATTER_DELIMITER { + return error("Document must start with YAML frontmatter (---)"); + } + + int i = 1; + while i < length && lines[i].trim() != FRONTMATTER_DELIMITER { + i += 1; + } + + if i >= length { + return error("Frontmatter is not closed (missing ---)"); + } + + string yamlContent = string:'join("\n", ...lines.slice(1, i)); + map frontmatter = check yaml:parseString(yamlContent); + string body = string:'join("\n", ...lines.slice(i + 1)); + return [frontmatter, body]; +} + function splitLines(string content) returns string[] { string[] result = []; - string remaining = content; + int length = content.length(); + int 'start = 0; - while true { - int? idx = remaining.indexOf("\n"); + while 'start < length { + int? idx = content.indexOf("\n", 'start); if idx is int { - result.push(remaining.substring(0, idx)); - remaining = remaining.substring(idx + 1); + result.push(content.substring('start, idx)); + 'start = idx + 1; } else { - if remaining.length() > 0 { - result.push(remaining); - } + result.push(content.substring('start)); break; } } diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index 3fd6971..065b820 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -15,7 +15,6 @@ // under the License. import ballerina/ai; -import ballerina/data.yaml; import ballerina/file; import ballerina/io; import ballerina/log; @@ -102,25 +101,8 @@ function parseSkillMd(string skillMdPath, string basePath) returns SkillInfo|err } function parseSkillMdContent(string content, string basePath, string[] resources) returns SkillInfo|error { - string[] lines = splitLines(content); - int length = lines.length(); - - if length == 0 || lines[0].trim() != FRONTMATTER_DELIMITER { - return error("SKILL.md must start with YAML frontmatter (---)"); - } - - int i = 1; - while i < length && lines[i].trim() != FRONTMATTER_DELIMITER { - i += 1; - } - - if i >= length { - return error("SKILL.md frontmatter is not closed (missing ---)"); - } - - string yamlContent = string:'join("\n", ...lines.slice(1, i)); - map intermediate = check yaml:parseString(yamlContent); - SkillFrontmatter frontmatter = check intermediate.fromJsonWithType(); + [map, string] [frontmatterMap, body] = check extractFrontmatter(content); + SkillFrontmatter frontmatter = check frontmatterMap.fromJsonWithType(); if frontmatter.name.trim() == "" { return error("SKILL.md 'name' field is required and must not be empty"); @@ -129,12 +111,10 @@ function parseSkillMdContent(string content, string basePath, string[] resources return error("SKILL.md 'description' field is required and must not be empty"); } - string body = string:'join("\n", ...lines.slice(i + 1)).trim(); - return { name: frontmatter.name, description: frontmatter.description, - body, + body: body.trim(), basePath, resources }; @@ -145,27 +125,19 @@ function listLocalResources(string basePath) returns string[] { foreach string dir in [REFERENCES_DIR, ASSETS_DIR] { do { string dirPath = check file:joinPath(basePath, dir); - if !check file:test(dirPath, file:EXISTS) { - continue; - } file:MetaData[] entries = check file:readDir(dirPath); foreach file:MetaData entry in entries { if !entry.dir { - resources.push(check file:joinPath(dir, getFileName(entry.absPath))); + resources.push(check file:joinPath(dir, check file:basename(entry.absPath))); } } } on fail error e { - log:printWarn(string `Failed to read directory ${dir}`, 'error = e); + log:printDebug(string `Failed to read directory ${dir}`, 'error = e); } } return resources; } -function getFileName(string path) returns string { - int? lastSlash = path.lastIndexOf("/") ?: path.lastIndexOf("\\"); - return lastSlash is int ? path.substring(lastSlash + 1) : path; -} - function buildSkillCatalog(map skills) returns string? { if skills.length() == 0 { return (); @@ -205,26 +177,27 @@ isolated class SkillsToolKit { # + return - the skill's full instructions and list of available resource files @ai:AgentTool isolated function activate_skill(string name) returns string|error { - lock { - if !self.skills.hasKey(name) { - return error(string `Skill '${name}' not found. Available skills: ${ - string:'join(", ", ...self.skills.keys())}`); - } + if !self.skills.hasKey(name) { + return error(string `Skill '${name}' not found. Available skills: ${ + string:'join(", ", ...self.skills.keys())}`); + } - SkillInfo info = self.skills.get(name); - return xml ` - -${info.body} -${info.resources.length() > 0 ? - xml ` + SkillInfo info = self.skills.get(name); + // Using string templates instead of XML literals to avoid escaping skill body content + // (e.g., angle brackets in code examples would be escaped to </> by XML) + string resourcesSection = info.resources.length() > 0 ? + string ` -${from string res in info.resources select xml `${res}`} +${string:'join("\n", ...from string res in info.resources select string `${res}`)} Use the read_skill_resource tool to read any of these files if needed. -` : xml ``} +` : ""; + return string ` + +${info.body} +${resourcesSection} -`.toString(); - } +`; } # Reads a resource file from a skill's references/ or assets/ directory. @@ -235,29 +208,27 @@ Use the read_skill_resource tool to read any of these files if needed. # + return - the content of the resource file @ai:AgentTool isolated function read_skill_resource(string skillName, string resourcePath) returns string|error { - lock { - if !self.skills.hasKey(skillName) { - return error(string `Skill '${skillName}' not found`); - } - - if !resourcePath.startsWith(string `${REFERENCES_DIR}/`) && - !resourcePath.startsWith(string `${ASSETS_DIR}/`) { - return error(string `Resource path must start with '${REFERENCES_DIR}/' or '${ASSETS_DIR}/'`); - } + if !self.skills.hasKey(skillName) { + return error(string `Skill '${skillName}' not found`); + } - if resourcePath.indexOf("../") != () || resourcePath.indexOf("..\\") != () { - return error("Path traversal is not allowed in resource paths"); - } + string[] segments = check file:splitPath(resourcePath); + if segments.length() < 2 || (segments[0] != REFERENCES_DIR && segments[0] != ASSETS_DIR) { + return error(string `Resource path must start with '${REFERENCES_DIR}/' or '${ASSETS_DIR}/'`); + } - SkillInfo info = self.skills.get(skillName); - if info.resources.indexOf(resourcePath) is () { - return error(string `Resource '${resourcePath}' not found in skill '${ - skillName}'. Available: ${string:'join(", ", ...info.resources)}`); - } + if resourcePath.indexOf("..") != () { + return error("Path traversal is not allowed in resource paths"); + } - string fullPath = check file:joinPath(info.basePath, resourcePath); - return io:fileReadString(fullPath); + SkillInfo info = self.skills.get(skillName); + if info.resources.indexOf(resourcePath) is () { + return error(string `Resource '${resourcePath}' not found in skill '${ + skillName}'. Available: ${string:'join(", ", ...info.resources)}`); } + + string fullPath = check file:joinPath(info.basePath, resourcePath); + return io:fileReadString(fullPath); } public isolated function getTools() returns ai:ToolConfig[] => diff --git a/ballerina-interpreter/tests/skills/duplicate_names/error_translator/SKILL.md b/ballerina-interpreter/tests/skills/duplicate_names/error_translator/SKILL.md new file mode 100644 index 0000000..81cc440 --- /dev/null +++ b/ballerina-interpreter/tests/skills/duplicate_names/error_translator/SKILL.md @@ -0,0 +1,8 @@ +--- +name: duplicate-skill +description: Translate error messages to user-friendly language +--- + +This is the first occurrence of the error-translator skill. + +Rewrite technical error messages so non-technical users can understand the problem and know what to do next. diff --git a/ballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.md b/ballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.md new file mode 100644 index 0000000..c7a94cd --- /dev/null +++ b/ballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.md @@ -0,0 +1,8 @@ +--- +name: duplicate-skill +description: Translate error messages (duplicate — should be skipped) +--- + +This is the duplicate occurrence of the error-translator skill. + +This is an intentional duplicate to test that the first occurrence wins. diff --git a/ballerina-interpreter/tests/skills/empty_dir/.gitkeep b/ballerina-interpreter/tests/skills/empty_dir/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/ballerina-interpreter/tests/skills/invalid_skill/SKILL.md b/ballerina-interpreter/tests/skills/invalid_skill/SKILL.md new file mode 100644 index 0000000..d46488d --- /dev/null +++ b/ballerina-interpreter/tests/skills/invalid_skill/SKILL.md @@ -0,0 +1,6 @@ +--- +name: "" +description: "" +--- + +This skill has empty required fields. diff --git a/ballerina-interpreter/tests/skills/multi_skills/pr_summary/SKILL.md b/ballerina-interpreter/tests/skills/multi_skills/pr_summary/SKILL.md new file mode 100644 index 0000000..b995c13 --- /dev/null +++ b/ballerina-interpreter/tests/skills/multi_skills/pr_summary/SKILL.md @@ -0,0 +1,12 @@ +--- +name: pr-summary +description: Summarize pull requests into concise changelog entries +--- + +Summarize each merged pull request into a single changelog line. + +When a PR is merged, produce a one-line changelog entry in the format: + +- ``: (#) + +Where type is one of: feat, fix, refactor, docs, test, chore. diff --git a/ballerina-interpreter/tests/skills/multi_skills/security_review/SKILL.md b/ballerina-interpreter/tests/skills/multi_skills/security_review/SKILL.md new file mode 100644 index 0000000..569e4be --- /dev/null +++ b/ballerina-interpreter/tests/skills/multi_skills/security_review/SKILL.md @@ -0,0 +1,15 @@ +--- +name: security-review +description: Review code for security vulnerabilities using OWASP guidelines +--- + +Perform a security-focused code review using OWASP guidelines. + +When reviewing code, check for: + +1. Injection flaws (SQL, command, path traversal) +2. Broken authentication or session management +3. Sensitive data exposure in logs or responses + +Consult the reference document for the full OWASP checklist. +Use the response template in assets/ to structure your findings. diff --git a/ballerina-interpreter/tests/skills/multi_skills/security_review/assets/template.json b/ballerina-interpreter/tests/skills/multi_skills/security_review/assets/template.json new file mode 100644 index 0000000..c55aa34 --- /dev/null +++ b/ballerina-interpreter/tests/skills/multi_skills/security_review/assets/template.json @@ -0,0 +1 @@ +{"template": "security-review", "version": 1, "sections": ["summary", "findings", "severity", "remediation"]} diff --git a/ballerina-interpreter/tests/skills/multi_skills/security_review/references/REFERENCE.md b/ballerina-interpreter/tests/skills/multi_skills/security_review/references/REFERENCE.md new file mode 100644 index 0000000..e2f3b02 --- /dev/null +++ b/ballerina-interpreter/tests/skills/multi_skills/security_review/references/REFERENCE.md @@ -0,0 +1,11 @@ +# OWASP Top 10 Quick Reference + +This is a reference file for security-review. + +| # | Category | +|---|---| +| A01 | Broken Access Control | +| A02 | Cryptographic Failures | +| A03 | Injection | +| A04 | Insecure Design | +| A05 | Security Misconfiguration | diff --git a/ballerina-interpreter/tests/skills/no_frontmatter/SKILL.md b/ballerina-interpreter/tests/skills/no_frontmatter/SKILL.md new file mode 100644 index 0000000..deafd5b --- /dev/null +++ b/ballerina-interpreter/tests/skills/no_frontmatter/SKILL.md @@ -0,0 +1,2 @@ +This file has no frontmatter at all. +Just plain content without any --- delimiters. diff --git a/ballerina-interpreter/tests/skills/partial_invalid/invalid_skill/SKILL.md b/ballerina-interpreter/tests/skills/partial_invalid/invalid_skill/SKILL.md new file mode 100644 index 0000000..b6992b1 --- /dev/null +++ b/ballerina-interpreter/tests/skills/partial_invalid/invalid_skill/SKILL.md @@ -0,0 +1,6 @@ +--- +name: "" +description: Invalid because name is empty +--- + +This skill has an empty name and should cause a parse error diff --git a/ballerina-interpreter/tests/skills/partial_invalid/valid_skill/SKILL.md b/ballerina-interpreter/tests/skills/partial_invalid/valid_skill/SKILL.md new file mode 100644 index 0000000..650cf63 --- /dev/null +++ b/ballerina-interpreter/tests/skills/partial_invalid/valid_skill/SKILL.md @@ -0,0 +1,6 @@ +--- +name: valid-skill +description: Convert JSON payloads to Markdown tables +--- + +Given a JSON array of objects, produce a Markdown table with column headers derived from the object keys. diff --git a/ballerina-interpreter/tests/skills/single_skill/SKILL.md b/ballerina-interpreter/tests/skills/single_skill/SKILL.md new file mode 100644 index 0000000..9a700da --- /dev/null +++ b/ballerina-interpreter/tests/skills/single_skill/SKILL.md @@ -0,0 +1,12 @@ +--- +name: test-gen +description: Generate and run unit tests for code changes +--- + +When asked to test code, follow these steps: + +1. Identify the functions or modules that changed. +2. Write unit tests covering the happy path and key edge cases. +3. Run the tests and report results. + +Keep tests focused — one assertion per behavior. This is the body of the test skill. diff --git a/ballerina-interpreter/tests/skills/source1/release_notes/SKILL.md b/ballerina-interpreter/tests/skills/source1/release_notes/SKILL.md new file mode 100644 index 0000000..f00d7d8 --- /dev/null +++ b/ballerina-interpreter/tests/skills/source1/release_notes/SKILL.md @@ -0,0 +1,8 @@ +--- +name: same-skill +description: Draft release notes from recent commits +--- + +This is from source1. + +Scan the commit log since the last tag and produce categorized release notes (Added, Changed, Fixed, Removed). diff --git a/ballerina-interpreter/tests/skills/source2/release_notes/SKILL.md b/ballerina-interpreter/tests/skills/source2/release_notes/SKILL.md new file mode 100644 index 0000000..4154a9b --- /dev/null +++ b/ballerina-interpreter/tests/skills/source2/release_notes/SKILL.md @@ -0,0 +1,8 @@ +--- +name: same-skill +description: Draft release notes (duplicate — should be skipped) +--- + +This is from source2. + +Intentional cross-source duplicate to verify first-source-wins behavior. diff --git a/ballerina-interpreter/tests/skills_test.bal b/ballerina-interpreter/tests/skills_test.bal new file mode 100644 index 0000000..bf1b877 --- /dev/null +++ b/ballerina-interpreter/tests/skills_test.bal @@ -0,0 +1,599 @@ +// Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +// +// WSO2 LLC. licenses this file to you under the Apache License, +// Version 2.0 (the "License"); you may not use this file except +// in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +import ballerina/file; +import ballerina/test; + +// ============================================ +// parseSkillMdContent — pure function tests +// ============================================ + +@test:Config +function testParseSkillMdContentValid() returns error? { + string content = string `--- +name: lint-fix +description: Auto-fix linting errors across the project +--- + +Run the configured linter and apply safe auto-fixes.`; + SkillInfo info = check parseSkillMdContent(content, "/base/path", ["references/doc.md"]); + test:assertEquals(info.name, "lint-fix"); + test:assertEquals(info.description, "Auto-fix linting errors across the project"); + test:assertEquals(info.body, "Run the configured linter and apply safe auto-fixes."); + test:assertEquals(info.basePath, "/base/path"); + test:assertEquals(info.resources, ["references/doc.md"]); +} + +@test:Config +function testParseSkillMdContentMissingFrontmatterOpener() { + string content = "No frontmatter here\nJust text."; + SkillInfo|error result = parseSkillMdContent(content, "/base", []); + if result is SkillInfo { + test:assertFail("Expected error for missing frontmatter"); + } + test:assertTrue(result.message().includes("must start with YAML frontmatter")); +} + +@test:Config +function testParseSkillMdContentUnclosedFrontmatter() { + string content = string `--- +name: deploy-check +description: Validate deployment readiness`; + SkillInfo|error result = parseSkillMdContent(content, "/base", []); + if result is SkillInfo { + test:assertFail("Expected error for unclosed frontmatter"); + } + test:assertTrue(result.message().includes("not closed")); +} + +@test:Config +function testParseSkillMdContentEmptyName() { + string content = string `--- +name: "" +description: Scaffold REST endpoints from OpenAPI specs +--- + +Body.`; + SkillInfo|error result = parseSkillMdContent(content, "/base", []); + if result is SkillInfo { + test:assertFail("Expected error for empty name"); + } + test:assertTrue(result.message().includes("'name' field is required")); +} + +@test:Config +function testParseSkillMdContentEmptyDescription() { + string content = string `--- +name: api-scaffold +description: "" +--- + +Body.`; + SkillInfo|error result = parseSkillMdContent(content, "/base", []); + if result is SkillInfo { + test:assertFail("Expected error for empty description"); + } + test:assertTrue(result.message().includes("'description' field is required")); +} + +@test:Config +function testParseSkillMdContentBodyWithResources() returns error? { + string content = string `--- +name: db-migrate +description: Generate and validate database migration scripts +--- + +Read the schema diff and produce migration SQL.`; + string[] resources = ["references/README.md", "assets/config.json"]; + SkillInfo info = check parseSkillMdContent(content, "/skills/db-migrate", resources); + test:assertEquals(info.resources, resources); + test:assertEquals(info.body, "Read the schema diff and produce migration SQL."); +} + +// ============================================ +// buildSkillCatalog — pure function tests +// ============================================ + +@test:Config +function testBuildSkillCatalogEmpty() { + map skills = {}; + string? catalog = buildSkillCatalog(skills); + test:assertTrue(catalog is ()); +} + +@test:Config +function testBuildSkillCatalogSingleSkill() { + map skills = { + "commit-msg": { + name: "commit-msg", + description: "Draft conventional commit messages from staged diffs", + body: "", + basePath: "", + resources: [] + } + }; + string? catalog = buildSkillCatalog(skills); + if catalog is () { + test:assertFail("Expected non-nil catalog"); + } + test:assertTrue(catalog.includes("commit-msg")); + test:assertTrue(catalog.includes("Draft conventional commit messages from staged diffs")); + test:assertTrue(catalog.includes("Available Skills")); +} + +@test:Config +function testBuildSkillCatalogMultipleSkills() { + map skills = { + "changelog": { + name: "changelog", + description: "Generate changelog entries from merged PRs", + body: "", + basePath: "", + resources: [] + }, + "dep-audit": { + name: "dep-audit", + description: "Audit dependencies for known vulnerabilities", + body: "", + basePath: "", + resources: [] + } + }; + string? catalog = buildSkillCatalog(skills); + if catalog is () { + test:assertFail("Expected non-nil catalog"); + } + test:assertTrue(catalog.includes("changelog")); + test:assertTrue(catalog.includes("dep-audit")); + test:assertTrue(catalog.includes("Generate changelog entries from merged PRs")); + test:assertTrue(catalog.includes("Audit dependencies for known vulnerabilities")); +} + +// ============================================ +// SkillsToolKit class tests +// ============================================ + +@test:Config +function testActivateSkillFound() returns error? { + map skills = { + "doc-gen": { + name: "doc-gen", + description: "Generate API documentation from source annotations", + body: "Scan exported symbols and produce Markdown API docs.", + basePath: "/skills/doc-gen", + resources: [] + } + }; + SkillsToolKit toolkit = new (skills); + string result = check toolkit.activate_skill("doc-gen"); + test:assertTrue(result.includes("Scan exported symbols and produce Markdown API docs.")); + test:assertTrue(result.includes("doc-gen")); +} + +@test:Config +function testActivateSkillFoundWithResources() returns error? { + map skills = { + "perf-profile": { + name: "perf-profile", + description: "Profile hot paths and suggest optimizations", + body: "Instrument the critical path and collect flame-graph data.", + basePath: "/skills/perf-profile", + resources: ["references/guide.md", "assets/schema.json"] + } + }; + SkillsToolKit toolkit = new (skills); + string result = check toolkit.activate_skill("perf-profile"); + test:assertTrue(result.includes("Instrument the critical path and collect flame-graph data.")); + test:assertTrue(result.includes("skill_resources")); + test:assertTrue(result.includes("references/guide.md")); + test:assertTrue(result.includes("assets/schema.json")); + test:assertTrue(result.includes("read_skill_resource")); +} + +@test:Config +function testActivateSkillNotFound() { + map skills = { + "format-sql": { + name: "format-sql", + description: "Format SQL queries to follow team style guide", + body: "Parse and reformat SQL using the project conventions.", + basePath: "/skills/format-sql", + resources: [] + } + }; + SkillsToolKit toolkit = new (skills); + string|error result = toolkit.activate_skill("nonexistent"); + if result is string { + test:assertFail("Expected error for nonexistent skill"); + } + test:assertTrue(result.message().includes("not found")); + test:assertTrue(result.message().includes("format-sql")); +} + +@test:Config +function testReadSkillResourceInvalidSkillName() { + map skills = {}; + SkillsToolKit toolkit = new (skills); + string|error result = toolkit.read_skill_resource("unknown", "references/file.md"); + if result is string { + test:assertFail("Expected error for unknown skill"); + } + test:assertTrue(result.message().includes("not found")); +} + +@test:Config +function testReadSkillResourceInvalidPathPrefix() { + map skills = { + "env-check": { + name: "env-check", + description: "Validate environment variables before deploy", + body: "Check required env vars.", + basePath: "/skills/env-check", + resources: ["references/env-spec.md"] + } + }; + SkillsToolKit toolkit = new (skills); + string|error result = toolkit.read_skill_resource("env-check", "other/file.txt"); + if result is string { + test:assertFail("Expected error for invalid path prefix"); + } + test:assertTrue(result.message().includes("must start with")); +} + +@test:Config +function testReadSkillResourcePathTraversal() { + map skills = { + "env-check": { + name: "env-check", + description: "Validate environment variables before deploy", + body: "Check required env vars.", + basePath: "/skills/env-check", + resources: ["references/env-spec.md"] + } + }; + SkillsToolKit toolkit = new (skills); + string|error result = toolkit.read_skill_resource("env-check", "references/../../../etc/passwd"); + if result is string { + test:assertFail("Expected error for path traversal"); + } + test:assertTrue(result.message().includes("traversal")); +} + +@test:Config +function testReadSkillResourceNotListed() { + map skills = { + "env-check": { + name: "env-check", + description: "Validate environment variables before deploy", + body: "Check required env vars.", + basePath: "/skills/env-check", + resources: ["references/env-spec.md"] + } + }; + SkillsToolKit toolkit = new (skills); + string|error result = toolkit.read_skill_resource("env-check", "references/other.md"); + if result is string { + test:assertFail("Expected error for unlisted resource"); + } + test:assertTrue(result.message().includes("not found in skill")); +} + +// ============================================ +// Filesystem-dependent tests (using fixtures) +// ============================================ + +@test:Config +function testDiscoverLocalSkillsSingleSkillDir() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/single_skill"); + map skills = check discoverLocalSkills(absPath); + test:assertEquals(skills.length(), 1); + test:assertTrue(skills.hasKey("test-gen")); + SkillInfo info = skills.get("test-gen"); + test:assertEquals(info.description, "Generate and run unit tests for code changes"); + test:assertTrue(info.body.includes("body of the test skill")); +} + +@test:Config +function testDiscoverLocalSkillsMultiSkillDir() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/multi_skills"); + map skills = check discoverLocalSkills(absPath); + test:assertEquals(skills.length(), 2); + test:assertTrue(skills.hasKey("pr-summary")); + test:assertTrue(skills.hasKey("security-review")); +} + +@test:Config +function testDiscoverLocalSkillsInvalidSkill() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/invalid_skill"); + map|error result = discoverLocalSkills(absPath); + // The skill has empty name, so parseSkillMdContent returns error. + // Since it's a direct SKILL.md (not subdirectory), the error propagates. + test:assertTrue(result is error); +} + +@test:Config +function testDiscoverSkillsMultipleSources() returns error? { + string testDir = check file:getAbsolutePath("tests/skills"); + SkillSource[] sources = [ + {path: "single_skill"}, + {path: "multi_skills"} + ]; + map skills = check discoverSkills(sources, testDir); + test:assertEquals(skills.length(), 3); + test:assertTrue(skills.hasKey("test-gen")); + test:assertTrue(skills.hasKey("pr-summary")); + test:assertTrue(skills.hasKey("security-review")); +} + +@test:Config +function testParseSkillMdValidFile() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/single_skill"); + string skillMdPath = check file:joinPath(absPath, "SKILL.md"); + SkillInfo info = check parseSkillMd(skillMdPath, absPath); + test:assertEquals(info.name, "test-gen"); + test:assertEquals(info.description, "Generate and run unit tests for code changes"); + test:assertTrue(info.body.includes("body of the test skill")); +} + +@test:Config +function testListLocalResourcesWithReferencesAndAssets() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/multi_skills/security_review"); + string[] resources = listLocalResources(absPath); + test:assertEquals(resources.length(), 2); + // Check that both references and assets are found + boolean hasReference = false; + boolean hasAsset = false; + foreach string res in resources { + if res.includes("references/") && res.includes("REFERENCE.md") { + hasReference = true; + } + if res.includes("assets/") && res.includes("template.json") { + hasAsset = true; + } + } + test:assertTrue(hasReference, "Should find references/REFERENCE.md"); + test:assertTrue(hasAsset, "Should find assets/template.json"); +} + +@test:Config +function testListLocalResourcesNoResources() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/single_skill"); + string[] resources = listLocalResources(absPath); + test:assertEquals(resources.length(), 0); +} + +@test:Config +function testExtractSkillCatalogNullSkills() returns error? { + AgentMetadata metadata = {}; + string|[string, SkillsToolKit]|error? result = extractSkillCatalog(metadata, "."); + test:assertTrue(result is ()); +} + +@test:Config +function testExtractSkillCatalogEmptySkillsArray() returns error? { + AgentMetadata metadata = {skills: []}; + string|[string, SkillsToolKit]|error? result = extractSkillCatalog(metadata, "."); + test:assertTrue(result is ()); +} + +// ============================================ +// Progressive disclosure E2E test +// ============================================ +// Walks the full chain using real fixtures: +// discover → catalog (names only) → activate (body revealed) → read resource (content revealed) + +@test:Config +function testProgressiveDisclosureEndToEnd() returns error? { + string testDir = check file:getAbsolutePath("tests/skills"); + + // Step 1: Discover skills from multi_skills fixture (has resources on security_review) + map skills = check discoverLocalSkills( + check file:joinPath(testDir, "multi_skills")); + test:assertEquals(skills.length(), 2); + + // Step 2: Build catalog — should contain ONLY names and descriptions, NOT bodies + string? catalog = buildSkillCatalog(skills); + if catalog is () { + test:assertFail("Catalog should be non-nil for discovered skills"); + } + + // Catalog MUST include names and descriptions + test:assertTrue(catalog.includes("pr-summary")); + test:assertTrue(catalog.includes("Summarize pull requests")); + test:assertTrue(catalog.includes("security-review")); + test:assertTrue(catalog.includes("Review code for security vulnerabilities")); + + // Catalog MUST NOT include skill bodies (progressive disclosure) + test:assertFalse(catalog.includes("Summarize each merged pull request"), + "Catalog must not leak pr-summary body"); + test:assertFalse(catalog.includes("Perform a security-focused code review"), + "Catalog must not leak security-review body"); + + // Step 3: Activate skill — body is now revealed + SkillsToolKit toolkit = new (skills); + string activateResult = check toolkit.activate_skill("security-review"); + + // Activation MUST include the full body + test:assertTrue(activateResult.includes("Perform a security-focused code review"), + "activate_skill must return the skill body"); + + // Activation MUST list available resources + test:assertTrue(activateResult.includes("skill_resources"), + "activate_skill must include skill_resources section"); + test:assertTrue(activateResult.includes("references/REFERENCE.md")); + test:assertTrue(activateResult.includes("assets/template.json")); + + // Step 4: Read a resource — file content is now revealed + string refContent = check toolkit.read_skill_resource("security-review", "references/REFERENCE.md"); + test:assertTrue(refContent.includes("OWASP Top 10 Quick Reference"), + "read_skill_resource must return actual file content"); + test:assertTrue(refContent.includes("reference file for security-review")); + + string assetContent = check toolkit.read_skill_resource("security-review", "assets/template.json"); + test:assertTrue(assetContent.includes("security-review"), + "read_skill_resource must return asset file content"); + + // Step 5: Verify non-resource skill has no resources disclosed + string alphaResult = check toolkit.activate_skill("pr-summary"); + test:assertTrue(alphaResult.includes("Summarize each merged pull request")); + test:assertFalse(alphaResult.includes("skill_resources"), + "pr-summary has no resources, so no skill_resources section"); +} + +@test:Config +function testProgressiveDisclosureViaExtractSkillCatalog() returns error? { + string testDir = check file:getAbsolutePath("tests/skills"); + AgentMetadata metadata = { + skills: [ + {path: "single_skill"}, + {path: "multi_skills"} + ] + }; + + // extractSkillCatalog returns [catalog, toolkit] — same entry point used by createAgent + var result = check extractSkillCatalog(metadata, testDir); + if result is () { + test:assertFail("Should return catalog and toolkit for valid skills"); + } + var [catalogStr, toolkit] = result; + + // Catalog has all 3 skills' names + test:assertTrue(catalogStr.includes("test-gen")); + test:assertTrue(catalogStr.includes("pr-summary")); + test:assertTrue(catalogStr.includes("security-review")); + + // Catalog does NOT have any body content + test:assertFalse(catalogStr.includes("body of the test skill"), + "Catalog must not contain test-gen body"); + test:assertFalse(catalogStr.includes("Summarize each merged pull request"), + "Catalog must not contain pr-summary body"); + test:assertFalse(catalogStr.includes("Perform a security-focused code review"), + "Catalog must not contain security-review body"); + + // Activating reveals the body + string body = check toolkit.activate_skill("test-gen"); + test:assertTrue(body.includes("body of the test skill"), + "activate_skill must reveal the full body"); +} + +// ============================================ +// Duplicate detection tests +// ============================================ + +@test:Config +function testDiscoverLocalSkillsDuplicateSubdirectories() returns error? { + // Test that duplicate skill names in subdirectories are detected + // First occurrence is kept, second is logged and skipped + string absPath = check file:getAbsolutePath("tests/skills/duplicate_names"); + map skills = check discoverLocalSkills(absPath); + + // Only one skill should be discovered despite two subdirectories with same name + test:assertEquals(skills.length(), 1); + test:assertTrue(skills.hasKey("duplicate-skill")); + + // First occurrence (error_translator) should be kept + SkillInfo info = skills.get("duplicate-skill"); + test:assertEquals(info.description, "Translate error messages to user-friendly language"); + test:assertTrue(info.body.includes("first occurrence")); +} + +@test:Config +function testDiscoverSkillsDuplicateFromMultipleSources() returns error? { + // Test that duplicate skill names across multiple sources are detected + // First source's skill is kept, subsequent duplicates are skipped (covers lines 54-55) + string testDir = check file:getAbsolutePath("tests/skills"); + SkillSource[] sources = [ + {path: "source1"}, // has "same-skill" + {path: "source2"} // also has "same-skill" (should be skipped) + ]; + + map skills = check discoverSkills(sources, testDir); + + // Should have only one skill (first source wins) + test:assertEquals(skills.length(), 1); + test:assertTrue(skills.hasKey("same-skill")); + + // Verify it's from source1 (first occurrence) + SkillInfo info = skills.get("same-skill"); + test:assertEquals(info.description, "Draft release notes from recent commits"); + test:assertTrue(info.body.includes("source1")); +} + +// ============================================ +// Error handling in subdirectory discovery +// ============================================ + +@test:Config +function testDiscoverLocalSkillsSkipsInvalidSubdir() returns error? { + // Test that invalid skills in subdirectories are skipped with error logging + // Valid skills in other subdirectories are still discovered + string absPath = check file:getAbsolutePath("tests/skills/partial_invalid"); + map skills = check discoverLocalSkills(absPath); + + // Should discover only the valid skill + test:assertEquals(skills.length(), 1); + test:assertTrue(skills.hasKey("valid-skill")); + test:assertFalse(skills.hasKey("")); // Invalid skill has empty name + + SkillInfo validInfo = skills.get("valid-skill"); + test:assertEquals(validInfo.description, "Convert JSON payloads to Markdown tables"); +} + +// ============================================ +// SkillsToolKit getTools test +// ============================================ + +@test:Config +function testSkillsToolKitGetTools() returns error? { + // Test that getTools() returns the correct tool configuration + map skills = { + "refactor": { + name: "refactor", + description: "Extract repeated logic into reusable functions", + body: "Identify duplication and extract helper functions.", + basePath: "/skills/refactor", + resources: [] + } + }; + + SkillsToolKit toolkit = new (skills); + var tools = toolkit.getTools(); + + // Should return 2 tools: activate_skill and read_skill_resource + test:assertEquals(tools.length(), 2); + + // Verify both tools are present by checking the function references + // The tools array contains ToolConfig records with the function metadata + test:assertTrue(tools.length() == 2, "Should have exactly 2 tools"); +} + +// ============================================ +// Empty skills directory test (line 36) +// ============================================ + +@test:Config +function testExtractSkillCatalogEmptyDirectory() returns error? { + AgentMetadata metadata = { + skills: [ + {path: "tests/skills/empty_dir"} + ] + }; + + [string, SkillsToolKit]|error? result = extractSkillCatalog(metadata, "."); + + // When no valid skills are found, should return () + test:assertTrue(result is (), "Should return () when no skills are found"); +} From 5a025fabec8cd9102ac6e2254074eea55c22e66d Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 13 Mar 2026 14:33:21 +0530 Subject: [PATCH 03/16] Refactor code --- ballerina-interpreter/skills.bal | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index 065b820..3908b8d 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -61,7 +61,6 @@ function discoverSkills(SkillSource[] sources, string afmFileDir) returns map|error { - map skills = {}; string absPath = check file:getAbsolutePath(path); string skillMdPath = check file:joinPath(absPath, SKILL_FILE); @@ -70,6 +69,7 @@ function discoverLocalSkills(string path) returns map|error { return {[info.name]: info}; } + map skills = {}; file:MetaData[] entries = check file:readDir(absPath); foreach file:MetaData entry in entries { if !entry.dir { @@ -217,7 +217,7 @@ ${resourcesSection} return error(string `Resource path must start with '${REFERENCES_DIR}/' or '${ASSETS_DIR}/'`); } - if resourcePath.indexOf("..") != () { + if segments.indexOf("..") != () { return error("Path traversal is not allowed in resource paths"); } From f5e2bb0ea376b872b57df7997610e631e35bacfa Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 13 Mar 2026 16:12:44 +0530 Subject: [PATCH 04/16] Address review suggestions --- ballerina-interpreter/skills.bal | 15 +++++++++++++-- ballerina-interpreter/tests/skills_test.bal | 4 ---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index 3908b8d..98a67ad 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -44,9 +44,17 @@ type SkillFrontmatter record { function discoverSkills(SkillSource[] sources, string afmFileDir) returns map|error { map skills = {}; + string normalizedAfmDir = check file:normalizePath(check file:getAbsolutePath(afmFileDir), file:CLEAN); foreach SkillSource 'source in sources { + if check file:isAbsolutePath('source.path) { + return error(string `Skill source path must be relative, but got: ${'source.path}`); + } string resolvedPath = check file:joinPath(afmFileDir, 'source.path); + string normalizedPath = check file:normalizePath(check file:getAbsolutePath(resolvedPath), file:CLEAN); + if !normalizedPath.startsWith(normalizedAfmDir) { + return error(string `Skill source path '${'source.path}' resolves outside the AFM file directory`); + } map localSkills = check discoverLocalSkills(resolvedPath); foreach [string, SkillInfo] [name, info] in localSkills.entries() { if skills.hasKey(name) { @@ -125,14 +133,17 @@ function listLocalResources(string basePath) returns string[] { foreach string dir in [REFERENCES_DIR, ASSETS_DIR] { do { string dirPath = check file:joinPath(basePath, dir); + if !check file:test(dirPath, file:EXISTS) { + continue; + } file:MetaData[] entries = check file:readDir(dirPath); foreach file:MetaData entry in entries { if !entry.dir { - resources.push(check file:joinPath(dir, check file:basename(entry.absPath))); + resources.push(string `${dir}/${check file:basename(entry.absPath)}`); } } } on fail error e { - log:printDebug(string `Failed to read directory ${dir}`, 'error = e); + log:printWarn(string `Failed to read directory ${dir}`, 'error = e); } } return resources; diff --git a/ballerina-interpreter/tests/skills_test.bal b/ballerina-interpreter/tests/skills_test.bal index bf1b877..54f4afc 100644 --- a/ballerina-interpreter/tests/skills_test.bal +++ b/ballerina-interpreter/tests/skills_test.bal @@ -574,10 +574,6 @@ function testSkillsToolKitGetTools() returns error? { // Should return 2 tools: activate_skill and read_skill_resource test:assertEquals(tools.length(), 2); - - // Verify both tools are present by checking the function references - // The tools array contains ToolConfig records with the function metadata - test:assertTrue(tools.length() == 2, "Should have exactly 2 tools"); } // ============================================ From 13fff5bf3d1b8404a3c5eb13c134f113ff824813 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Sat, 14 Mar 2026 12:14:01 +0530 Subject: [PATCH 05/16] Make metadata optional --- ballerina-interpreter/main.bal | 4 ++-- ballerina-interpreter/parser.bal | 18 +++++++++++++++--- ballerina-interpreter/skills.bal | 2 +- ballerina-interpreter/tests/main_test.bal | 12 ++++++------ ballerina-interpreter/types.bal | 2 +- ballerina-interpreter/web_chat_ui.bal | 8 ++++---- 6 files changed, 29 insertions(+), 17 deletions(-) diff --git a/ballerina-interpreter/main.bal b/ballerina-interpreter/main.bal index ebadc8f..c43119a 100644 --- a/ballerina-interpreter/main.bal +++ b/ballerina-interpreter/main.bal @@ -50,9 +50,9 @@ public function main(string? filePath = ()) returns error? { } function runAgentFromAFM(AFMRecord afm, int port, string afmFileDir) returns error? { - AgentMetadata metadata = afm.metadata; + AgentMetadata? metadata = afm?.metadata; - Interface[] agentInterfaces = metadata.interfaces ?: [{}]; + Interface[] agentInterfaces = metadata?.interfaces ?: [{}]; var [consoleChatInterface, webChatInterface, webhookInterface] = check validateAndExtractInterfaces(agentInterfaces); diff --git a/ballerina-interpreter/parser.bal b/ballerina-interpreter/parser.bal index d12296e..3158c80 100644 --- a/ballerina-interpreter/parser.bal +++ b/ballerina-interpreter/parser.bal @@ -20,8 +20,15 @@ import ballerina/os; function parseAfm(string content) returns AFMRecord|error { string resolvedContent = check resolveVariables(content); - [map, string] [frontmatterMap, body] = check extractFrontmatter(resolvedContent); - AgentMetadata metadata = check frontmatterMap.fromJsonWithType(); + AgentMetadata? metadata = (); + string body; + if resolvedContent.startsWith(FRONTMATTER_DELIMITER) { + map frontmatterMap; + [frontmatterMap, body] = check extractFrontmatter(resolvedContent); + metadata = check frontmatterMap.fromJsonWithType(); + } else { + body = resolvedContent; + } // Extract Role and Instructions sections string[] bodyLines = splitLines(body); @@ -140,7 +147,12 @@ function validateHttpVariables(AFMRecord afmRecord) returns error? { return error("http: variables are only supported in webhook prompt fields, found in instructions section"); } - AgentMetadata {authors, provider, model, interfaces, tools, skills, max_iterations: _, ...rest} = afmRecord.metadata; + AgentMetadata? metadata = afmRecord?.metadata; + if metadata is () { + return; + } + + AgentMetadata {authors, provider, model, interfaces, tools, skills, max_iterations: _, ...rest} = metadata; string[] erroredKeys = []; diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index 98a67ad..feb6d10 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -23,7 +23,7 @@ const SKILL_FILE = "SKILL.md"; const REFERENCES_DIR = "references"; const ASSETS_DIR = "assets"; -function extractSkillCatalog(AgentMetadata metadata, string afmFileDir) returns [string, SkillsToolKit]|error? { +function extractSkillCatalog(AgentMetadata? metadata, string afmFileDir) returns [string, SkillsToolKit]|error? { SkillSource[]? skillSources = metadata?.skills; if skillSources is () || skillSources.length() == 0 { return (); diff --git a/ballerina-interpreter/tests/main_test.bal b/ballerina-interpreter/tests/main_test.bal index 3df4ac9..e9ad324 100644 --- a/ballerina-interpreter/tests/main_test.bal +++ b/ballerina-interpreter/tests/main_test.bal @@ -251,17 +251,17 @@ function testValidateHttpVariablesInStdioTransportEnv() { } @test:Config -function testParseAfmWithoutFrontmatter() { +function testParseAfmWithoutFrontmatter() returns error? { string content = string `# Role This is the role. # Instructions These are instructions.`; - AFMRecord|error parsed = parseAfm(content); - if parsed is AFMRecord { - test:assertFail("Expected error when parsing AFM without frontmatter"); - } + AFMRecord parsed = check parseAfm(content); + test:assertTrue(parsed?.metadata is (), "metadata should be absent"); + test:assertEquals(parsed.role, "This is the role."); + test:assertEquals(parsed.instructions, "These are instructions."); } @test:Config @@ -277,7 +277,7 @@ Agent role here. Agent instructions here.`; AFMRecord parsed = check parseAfm(content); - test:assertEquals(parsed.metadata.spec_version, "0.3.0"); + test:assertEquals(parsed?.metadata?.spec_version, "0.3.0"); test:assertEquals(parsed.role, "Agent role here."); test:assertEquals(parsed.instructions, "Agent instructions here."); } diff --git a/ballerina-interpreter/types.bal b/ballerina-interpreter/types.bal index 77ced96..392e409 100644 --- a/ballerina-interpreter/types.bal +++ b/ballerina-interpreter/types.bal @@ -174,7 +174,7 @@ type AgentMetadata record {| |}; type AFMRecord record {| - AgentMetadata metadata; + AgentMetadata metadata?; string role; string instructions; |}; diff --git a/ballerina-interpreter/web_chat_ui.bal b/ballerina-interpreter/web_chat_ui.bal index 36fd1bb..50b60a0 100644 --- a/ballerina-interpreter/web_chat_ui.bal +++ b/ballerina-interpreter/web_chat_ui.bal @@ -17,7 +17,7 @@ import ballerina/http; import ballerina/io; -function attachWebChatUIService(http:Listener httpListener, string chatPath, AgentMetadata metadata) returns error? { +function attachWebChatUIService(http:Listener httpListener, string chatPath, AgentMetadata? metadata) returns error? { string absoluteChatPath = chatPath.startsWith("/") ? chatPath : "/" + chatPath; http:Service webUIService = check new WebUIService(absoluteChatPath, metadata); return httpListener.attach(webUIService, "/chat/ui"); @@ -28,11 +28,11 @@ service class WebUIService { private final string htmlContent; - function init(string chatPath, AgentMetadata metadata) returns error? { + function init(string chatPath, AgentMetadata? metadata) returns error? { string template = check io:fileReadString("resources/chat-ui.html"); - string agentName = metadata.name ?: "AFM Agent Chat"; - string agentDescription = metadata.description ?: "AI Assistant"; + string agentName = metadata?.name ?: "AFM Agent Chat"; + string agentDescription = metadata?.description ?: "AI Assistant"; string escapedPath = escapeForJavaScript(chatPath); string result = template; From 8f5080bd8c6bf59aa5a56cf9c6d3f9cad0e25f7b Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 16 Mar 2026 10:40:22 +0530 Subject: [PATCH 06/16] Refactor code --- ballerina-interpreter/parser.bal | 7 ++-- ballerina-interpreter/skills.bal | 26 +++++++------- ballerina-interpreter/tests/skills_test.bal | 38 ++++++++++----------- 3 files changed, 36 insertions(+), 35 deletions(-) diff --git a/ballerina-interpreter/parser.bal b/ballerina-interpreter/parser.bal index 3158c80..8657ffb 100644 --- a/ballerina-interpreter/parser.bal +++ b/ballerina-interpreter/parser.bal @@ -24,7 +24,7 @@ function parseAfm(string content) returns AFMRecord|error { string body; if resolvedContent.startsWith(FRONTMATTER_DELIMITER) { map frontmatterMap; - [frontmatterMap, body] = check extractFrontmatter(resolvedContent); + [frontmatterMap, body] = check extractFrontMatter(resolvedContent); metadata = check frontmatterMap.fromJsonWithType(); } else { body = resolvedContent; @@ -37,8 +37,7 @@ function parseAfm(string content) returns AFMRecord|error { boolean inRole = false; boolean inInstructions = false; - foreach int k in 0 ..< bodyLines.length() { - string line = bodyLines[k]; + foreach string line in bodyLines { string trimmed = line.trim(); if trimmed.startsWith("# ") { @@ -399,7 +398,7 @@ function toolFilterContainsHttpVariable(ToolFilter? filter) returns boolean { // Extracts YAML frontmatter and the remaining body from a document delimited by `---`. // Returns the parsed YAML as a map and the body text after the closing delimiter. -function extractFrontmatter(string content) returns [map, string]|error { +function extractFrontMatter(string content) returns [map, string]|error { string[] lines = splitLines(content); int length = lines.length(); diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index feb6d10..f555ae3 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -105,23 +105,25 @@ function discoverLocalSkills(string path) returns map|error { function parseSkillMd(string skillMdPath, string basePath) returns SkillInfo|error { string content = check io:fileReadString(skillMdPath); - return parseSkillMdContent(content, basePath, listLocalResources(basePath)); + string[] resources = listLocalResources(basePath); + return parseSkillMdContent(content, basePath, resources); } function parseSkillMdContent(string content, string basePath, string[] resources) returns SkillInfo|error { - [map, string] [frontmatterMap, body] = check extractFrontmatter(content); - SkillFrontmatter frontmatter = check frontmatterMap.fromJsonWithType(); + [map, string] [frontMatterMap, body] = check extractFrontMatter(content); + SkillFrontmatter {name, description} = check frontMatterMap.fromJsonWithType(); - if frontmatter.name.trim() == "" { + if name.trim() == "" { return error("SKILL.md 'name' field is required and must not be empty"); } - if frontmatter.description.trim() == "" { + + if description.trim() == "" { return error("SKILL.md 'description' field is required and must not be empty"); } return { - name: frontmatter.name, - description: frontmatter.description, + name, + description, body: body.trim(), basePath, resources @@ -158,7 +160,7 @@ function buildSkillCatalog(map skills) returns string? { ## Available Skills The following skills provide specialized instructions for specific tasks. -When a task matches a skill's description, call the activate_skill tool +When a task matches a skill's description, call the activateSkill tool with the skill's name to load its full instructions. ${xml ` @@ -187,7 +189,7 @@ isolated class SkillsToolKit { # + name - the name of the skill to activate (must match a name from the available skills catalog) # + return - the skill's full instructions and list of available resource files @ai:AgentTool - isolated function activate_skill(string name) returns string|error { + isolated function activateSkill(string name) returns string|error { if !self.skills.hasKey(name) { return error(string `Skill '${name}' not found. Available skills: ${ string:'join(", ", ...self.skills.keys())}`); @@ -201,7 +203,7 @@ isolated class SkillsToolKit { ${string:'join("\n", ...from string res in info.resources select string `${res}`)} -Use the read_skill_resource tool to read any of these files if needed. +Use the readSkillResource tool to read any of these files if needed. ` : ""; return string ` @@ -218,7 +220,7 @@ ${resourcesSection} # + resourcePath - relative path to the resource file (e.g., "references/REFERENCE.md" or "assets/template.json") # + return - the content of the resource file @ai:AgentTool - isolated function read_skill_resource(string skillName, string resourcePath) returns string|error { + isolated function readSkillResource(string skillName, string resourcePath) returns string|error { if !self.skills.hasKey(skillName) { return error(string `Skill '${skillName}' not found`); } @@ -243,5 +245,5 @@ ${resourcesSection} } public isolated function getTools() returns ai:ToolConfig[] => - ai:getToolConfigs([self.activate_skill, self.read_skill_resource]); + ai:getToolConfigs([self.activateSkill, self.readSkillResource]); } diff --git a/ballerina-interpreter/tests/skills_test.bal b/ballerina-interpreter/tests/skills_test.bal index 54f4afc..d9622d2 100644 --- a/ballerina-interpreter/tests/skills_test.bal +++ b/ballerina-interpreter/tests/skills_test.bal @@ -178,7 +178,7 @@ function testActivateSkillFound() returns error? { } }; SkillsToolKit toolkit = new (skills); - string result = check toolkit.activate_skill("doc-gen"); + string result = check toolkit.activateSkill("doc-gen"); test:assertTrue(result.includes("Scan exported symbols and produce Markdown API docs.")); test:assertTrue(result.includes("doc-gen")); } @@ -195,12 +195,12 @@ function testActivateSkillFoundWithResources() returns error? { } }; SkillsToolKit toolkit = new (skills); - string result = check toolkit.activate_skill("perf-profile"); + string result = check toolkit.activateSkill("perf-profile"); test:assertTrue(result.includes("Instrument the critical path and collect flame-graph data.")); test:assertTrue(result.includes("skill_resources")); test:assertTrue(result.includes("references/guide.md")); test:assertTrue(result.includes("assets/schema.json")); - test:assertTrue(result.includes("read_skill_resource")); + test:assertTrue(result.includes("readSkillResource")); } @test:Config @@ -215,7 +215,7 @@ function testActivateSkillNotFound() { } }; SkillsToolKit toolkit = new (skills); - string|error result = toolkit.activate_skill("nonexistent"); + string|error result = toolkit.activateSkill("nonexistent"); if result is string { test:assertFail("Expected error for nonexistent skill"); } @@ -227,7 +227,7 @@ function testActivateSkillNotFound() { function testReadSkillResourceInvalidSkillName() { map skills = {}; SkillsToolKit toolkit = new (skills); - string|error result = toolkit.read_skill_resource("unknown", "references/file.md"); + string|error result = toolkit.readSkillResource("unknown", "references/file.md"); if result is string { test:assertFail("Expected error for unknown skill"); } @@ -246,7 +246,7 @@ function testReadSkillResourceInvalidPathPrefix() { } }; SkillsToolKit toolkit = new (skills); - string|error result = toolkit.read_skill_resource("env-check", "other/file.txt"); + string|error result = toolkit.readSkillResource("env-check", "other/file.txt"); if result is string { test:assertFail("Expected error for invalid path prefix"); } @@ -265,7 +265,7 @@ function testReadSkillResourcePathTraversal() { } }; SkillsToolKit toolkit = new (skills); - string|error result = toolkit.read_skill_resource("env-check", "references/../../../etc/passwd"); + string|error result = toolkit.readSkillResource("env-check", "references/../../../etc/passwd"); if result is string { test:assertFail("Expected error for path traversal"); } @@ -284,7 +284,7 @@ function testReadSkillResourceNotListed() { } }; SkillsToolKit toolkit = new (skills); - string|error result = toolkit.read_skill_resource("env-check", "references/other.md"); + string|error result = toolkit.readSkillResource("env-check", "references/other.md"); if result is string { test:assertFail("Expected error for unlisted resource"); } @@ -424,30 +424,30 @@ function testProgressiveDisclosureEndToEnd() returns error? { // Step 3: Activate skill — body is now revealed SkillsToolKit toolkit = new (skills); - string activateResult = check toolkit.activate_skill("security-review"); + string activateResult = check toolkit.activateSkill("security-review"); // Activation MUST include the full body test:assertTrue(activateResult.includes("Perform a security-focused code review"), - "activate_skill must return the skill body"); + "activateSkill must return the skill body"); // Activation MUST list available resources test:assertTrue(activateResult.includes("skill_resources"), - "activate_skill must include skill_resources section"); + "activateSkill must include skill_resources section"); test:assertTrue(activateResult.includes("references/REFERENCE.md")); test:assertTrue(activateResult.includes("assets/template.json")); // Step 4: Read a resource — file content is now revealed - string refContent = check toolkit.read_skill_resource("security-review", "references/REFERENCE.md"); + string refContent = check toolkit.readSkillResource("security-review", "references/REFERENCE.md"); test:assertTrue(refContent.includes("OWASP Top 10 Quick Reference"), - "read_skill_resource must return actual file content"); + "readSkillResource must return actual file content"); test:assertTrue(refContent.includes("reference file for security-review")); - string assetContent = check toolkit.read_skill_resource("security-review", "assets/template.json"); + string assetContent = check toolkit.readSkillResource("security-review", "assets/template.json"); test:assertTrue(assetContent.includes("security-review"), - "read_skill_resource must return asset file content"); + "readSkillResource must return asset file content"); // Step 5: Verify non-resource skill has no resources disclosed - string alphaResult = check toolkit.activate_skill("pr-summary"); + string alphaResult = check toolkit.activateSkill("pr-summary"); test:assertTrue(alphaResult.includes("Summarize each merged pull request")); test:assertFalse(alphaResult.includes("skill_resources"), "pr-summary has no resources, so no skill_resources section"); @@ -484,9 +484,9 @@ function testProgressiveDisclosureViaExtractSkillCatalog() returns error? { "Catalog must not contain security-review body"); // Activating reveals the body - string body = check toolkit.activate_skill("test-gen"); + string body = check toolkit.activateSkill("test-gen"); test:assertTrue(body.includes("body of the test skill"), - "activate_skill must reveal the full body"); + "activateSkill must reveal the full body"); } // ============================================ @@ -572,7 +572,7 @@ function testSkillsToolKitGetTools() returns error? { SkillsToolKit toolkit = new (skills); var tools = toolkit.getTools(); - // Should return 2 tools: activate_skill and read_skill_resource + // Should return 2 tools: activateSkill and readSkillResource test:assertEquals(tools.length(), 2); } From 8c91e45d225826cf7e9f38a1d27bbff72793a710 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 16 Mar 2026 14:59:33 +0530 Subject: [PATCH 07/16] Fix path check --- ballerina-interpreter/skills.bal | 2 +- ballerina-interpreter/tests/skills_test.bal | 38 +++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index f555ae3..e535013 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -52,7 +52,7 @@ function discoverSkills(SkillSource[] sources, string afmFileDir) returns map localSkills = check discoverLocalSkills(resolvedPath); diff --git a/ballerina-interpreter/tests/skills_test.bal b/ballerina-interpreter/tests/skills_test.bal index d9622d2..c8e070e 100644 --- a/ballerina-interpreter/tests/skills_test.bal +++ b/ballerina-interpreter/tests/skills_test.bal @@ -324,6 +324,44 @@ function testDiscoverLocalSkillsInvalidSkill() returns error? { test:assertTrue(result is error); } +@test:Config +function testDiscoverSkillsAbsolutePathRejected() returns error? { + string testDir = check file:getAbsolutePath("tests/skills"); + SkillSource[] sources = [{path: "/absolute/path/to/skills"}]; + map|error result = discoverSkills(sources, testDir); + if result is map { + test:assertFail("Expected error for absolute path"); + } + test:assertTrue(result.message().includes("must be relative")); +} + +@test:Config +function testDiscoverSkillsPathOutsideAfmDirRejected() returns error? { + string testDir = check file:getAbsolutePath("tests/skills"); + SkillSource[] sources = [{path: "../../outside"}]; + map|error result = discoverSkills(sources, testDir); + if result is map { + test:assertFail("Expected error for path outside AFM directory"); + } + test:assertTrue(result.message().includes("resolves outside")); +} + +@test:Config +function testDiscoverSkillsSiblingPrefixPathRejected() returns error? { + // A sibling directory sharing a name prefix must not pass validation. + // E.g. if afm_dir is '/a/skills', a path resolving to '/a/skills-evil' + // should be rejected even though it starts with the same string prefix. + string testDir = check file:getAbsolutePath("tests/skills/single_skill"); + // "../single_skill_extra" would resolve to tests/skills/single_skill_extra + // which shares the prefix "tests/skills/single_skill" but is outside the dir + SkillSource[] sources = [{path: "../single_skill_extra"}]; + map|error result = discoverSkills(sources, testDir); + if result is map { + test:assertFail("Expected error for sibling prefix path"); + } + test:assertTrue(result.message().includes("resolves outside")); +} + @test:Config function testDiscoverSkillsMultipleSources() returns error? { string testDir = check file:getAbsolutePath("tests/skills"); From 608ce9ce4cda747cc50454930e6d7ab057d02fb8 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 16 Mar 2026 15:21:09 +0530 Subject: [PATCH 08/16] Fix template --- templates/agent_template.afm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/agent_template.afm.md b/templates/agent_template.afm.md index b31b8ce..78e78c3 100644 --- a/templates/agent_template.afm.md +++ b/templates/agent_template.afm.md @@ -159,7 +159,7 @@ tools: # SKILLS - OPTIONAL (Agent Skills format: https://agentskills.io) # ============================================================================ skills: - # Local skills directory or (a directory that may contain multiple skill subdirectories) + # Local skills directory (or a directory that may contain multiple skill subdirectories) - type: "local" path: "./skills" --- From f97c1a306a3edc952dff6bd7a4f162c5c27136ea Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 19 Mar 2026 10:21:49 +0530 Subject: [PATCH 09/16] Allow absolute paths for skill dirs --- ballerina-interpreter/skills.bal | 12 ++---- ballerina-interpreter/tests/skills_test.bal | 41 +++------------------ 2 files changed, 9 insertions(+), 44 deletions(-) diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index e535013..2c84ffc 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -44,17 +44,11 @@ type SkillFrontmatter record { function discoverSkills(SkillSource[] sources, string afmFileDir) returns map|error { map skills = {}; - string normalizedAfmDir = check file:normalizePath(check file:getAbsolutePath(afmFileDir), file:CLEAN); foreach SkillSource 'source in sources { - if check file:isAbsolutePath('source.path) { - return error(string `Skill source path must be relative, but got: ${'source.path}`); - } - string resolvedPath = check file:joinPath(afmFileDir, 'source.path); - string normalizedPath = check file:normalizePath(check file:getAbsolutePath(resolvedPath), file:CLEAN); - if normalizedPath != normalizedAfmDir && !normalizedPath.startsWith(normalizedAfmDir + file:pathSeparator) { - return error(string `Skill source path '${'source.path}' resolves outside the AFM file directory`); - } + string resolvedPath = check file:isAbsolutePath('source.path) + ? 'source.path + : check file:joinPath(afmFileDir, 'source.path); map localSkills = check discoverLocalSkills(resolvedPath); foreach [string, SkillInfo] [name, info] in localSkills.entries() { if skills.hasKey(name) { diff --git a/ballerina-interpreter/tests/skills_test.bal b/ballerina-interpreter/tests/skills_test.bal index c8e070e..33268ec 100644 --- a/ballerina-interpreter/tests/skills_test.bal +++ b/ballerina-interpreter/tests/skills_test.bal @@ -325,41 +325,12 @@ function testDiscoverLocalSkillsInvalidSkill() returns error? { } @test:Config -function testDiscoverSkillsAbsolutePathRejected() returns error? { - string testDir = check file:getAbsolutePath("tests/skills"); - SkillSource[] sources = [{path: "/absolute/path/to/skills"}]; - map|error result = discoverSkills(sources, testDir); - if result is map { - test:assertFail("Expected error for absolute path"); - } - test:assertTrue(result.message().includes("must be relative")); -} - -@test:Config -function testDiscoverSkillsPathOutsideAfmDirRejected() returns error? { - string testDir = check file:getAbsolutePath("tests/skills"); - SkillSource[] sources = [{path: "../../outside"}]; - map|error result = discoverSkills(sources, testDir); - if result is map { - test:assertFail("Expected error for path outside AFM directory"); - } - test:assertTrue(result.message().includes("resolves outside")); -} - -@test:Config -function testDiscoverSkillsSiblingPrefixPathRejected() returns error? { - // A sibling directory sharing a name prefix must not pass validation. - // E.g. if afm_dir is '/a/skills', a path resolving to '/a/skills-evil' - // should be rejected even though it starts with the same string prefix. - string testDir = check file:getAbsolutePath("tests/skills/single_skill"); - // "../single_skill_extra" would resolve to tests/skills/single_skill_extra - // which shares the prefix "tests/skills/single_skill" but is outside the dir - SkillSource[] sources = [{path: "../single_skill_extra"}]; - map|error result = discoverSkills(sources, testDir); - if result is map { - test:assertFail("Expected error for sibling prefix path"); - } - test:assertTrue(result.message().includes("resolves outside")); +function testDiscoverSkillsAbsolutePathAccepted() returns error? { + string absPath = check file:getAbsolutePath("tests/skills/single_skill"); + SkillSource[] sources = [{path: absPath}]; + map skills = check discoverSkills(sources, "some/other/dir"); + test:assertEquals(skills.length(), 1); + test:assertTrue(skills.hasKey("test-gen")); } @test:Config From 3754b8e5bda634cbaf08879ef7968bc1b49d830c Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 13 Mar 2026 15:36:38 +0530 Subject: [PATCH 10/16] Support Agent Skills with the Python Interpreter --- .../packages/afm-core/src/afm/models.py | 26 ++ .../packages/afm-core/src/afm/parser.py | 47 ++- .../packages/afm-core/src/afm/skills.py | 252 ++++++++++++++ .../tests/fixtures/skills/empty_dir/.gitkeep | 0 .../fixtures/skills/invalid_skill/SKILL.md | 6 + .../skills/multi_skills/pr_summary/SKILL.md | 12 + .../multi_skills/security_review/SKILL.md | 15 + .../security_review/assets/template.json | 1 + .../security_review/references/REFERENCE.md | 11 + .../fixtures/skills/no_frontmatter/SKILL.md | 1 + .../fixtures/skills/single_skill/SKILL.md | 12 + .../packages/afm-core/tests/test_skills.py | 329 ++++++++++++++++++ .../src/afm_langchain/backend.py | 24 +- .../src/afm_langchain/tools/skills.py | 88 +++++ .../tests/test_langchain_skills.py | 169 +++++++++ 15 files changed, 988 insertions(+), 5 deletions(-) create mode 100644 python-interpreter/packages/afm-core/src/afm/skills.py create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/empty_dir/.gitkeep create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/invalid_skill/SKILL.md create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/pr_summary/SKILL.md create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/SKILL.md create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/assets/template.json create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/references/REFERENCE.md create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/no_frontmatter/SKILL.md create mode 100644 python-interpreter/packages/afm-core/tests/fixtures/skills/single_skill/SKILL.md create mode 100644 python-interpreter/packages/afm-core/tests/test_skills.py create mode 100644 python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py create mode 100644 python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py diff --git a/python-interpreter/packages/afm-core/src/afm/models.py b/python-interpreter/packages/afm-core/src/afm/models.py index e9b4f03..4772a98 100644 --- a/python-interpreter/packages/afm-core/src/afm/models.py +++ b/python-interpreter/packages/afm-core/src/afm/models.py @@ -17,6 +17,7 @@ from __future__ import annotations from enum import Enum +from pathlib import Path from typing import Annotated, Literal, Self from pydantic import BaseModel, ConfigDict, Field, model_validator @@ -193,6 +194,29 @@ class WebhookInterface(BaseModel): ] +class LocalSkillSource(BaseModel): + model_config = ConfigDict(extra="forbid") + + type: Literal["local"] = "local" + path: str + + +SkillSource = Annotated[ + LocalSkillSource, + Field(discriminator="type"), +] + + +class SkillInfo(BaseModel): + model_config = ConfigDict(extra="forbid") + + name: str + description: str + body: str + base_path: Path + resources: list[str] = Field(default_factory=list) + + class AgentMetadata(BaseModel): model_config = ConfigDict(extra="forbid") @@ -208,6 +232,7 @@ class AgentMetadata(BaseModel): model: Model | None = None interfaces: list[Interface] | None = None tools: Tools | None = None + skills: list[SkillSource] | None = None max_iterations: int | None = None @@ -217,6 +242,7 @@ class AFMRecord(BaseModel): metadata: AgentMetadata role: str instructions: str + source_dir: Path | None = None class LiteralSegment(BaseModel): diff --git a/python-interpreter/packages/afm-core/src/afm/parser.py b/python-interpreter/packages/afm-core/src/afm/parser.py index 24ad536..f5bfede 100644 --- a/python-interpreter/packages/afm-core/src/afm/parser.py +++ b/python-interpreter/packages/afm-core/src/afm/parser.py @@ -27,6 +27,47 @@ FRONTMATTER_DELIMITER = "---" +def extract_raw_frontmatter(content: str) -> tuple[dict | None, str]: + """Extract raw YAML frontmatter dict and remaining body from a content string. + + Returns ``(None, content)`` when no opening delimiter is found. + Returns ``(dict, body)`` when frontmatter is present and parsed. + Raises :class:`ValueError` on malformed frontmatter. + """ + lines = content.splitlines() + + if not lines or lines[0].strip() != FRONTMATTER_DELIMITER: + return None, content + + end_index = None + for i in range(1, len(lines)): + if lines[i].strip() == FRONTMATTER_DELIMITER: + end_index = i + break + + if end_index is None: + raise ValueError("Unclosed frontmatter - missing closing '---'") + + yaml_content = "\n".join(lines[1:end_index]) + body = "\n".join(lines[end_index + 1 :]) + + if not yaml_content.strip(): + return {}, body + + try: + yaml_data = yaml.safe_load(yaml_content) + except yaml.YAMLError as e: + raise ValueError(f"Invalid YAML in frontmatter: {e}") from e + + if yaml_data is None: + return {}, body + + if not isinstance(yaml_data, dict): + raise ValueError("Frontmatter must be a YAML mapping") + + return yaml_data, body + + def parse_afm(content: str, *, resolve_env: bool = True) -> AFMRecord: if resolve_env: content = resolve_variables(content) @@ -43,9 +84,11 @@ def parse_afm(content: str, *, resolve_env: bool = True) -> AFMRecord: def parse_afm_file(file_path: str | Path, *, resolve_env: bool = True) -> AFMRecord: - path = Path(file_path) + path = Path(file_path).resolve() content = path.read_text(encoding="utf-8") - return parse_afm(content, resolve_env=resolve_env) + record = parse_afm(content, resolve_env=resolve_env) + record.source_dir = path.parent + return record def _extract_frontmatter(lines: list[str]) -> tuple[AgentMetadata, int]: diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py new file mode 100644 index 0000000..23c2a33 --- /dev/null +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -0,0 +1,252 @@ +# Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +# +# WSO2 LLC. licenses this file to you under the Apache License, +# Version 2.0 (the "License"); you may not use this file except +# in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +import logging +from pathlib import Path +from xml.sax.saxutils import escape as xml_escape + +from .models import AgentMetadata, SkillInfo, SkillSource +from .parser import extract_raw_frontmatter + +logger = logging.getLogger(__name__) + +SKILL_FILE = "SKILL.md" +REFERENCES_DIR = "references" +ASSETS_DIR = "assets" + + +def extract_skill_catalog( + metadata: AgentMetadata, afm_file_dir: Path +) -> tuple[str, dict[str, SkillInfo]] | None: + """Discover skills and build a catalog string. + + Returns a tuple of (catalog_text, skills_dict) or None if no skills found. + """ + if not metadata.skills: + return None + + skills = discover_skills(metadata.skills, afm_file_dir) + if not skills: + return None + + logger.debug( + "Loaded %d skill(s): %s", len(skills), ", ".join(skills.keys()) + ) + + catalog = build_skill_catalog(skills) + if catalog is None: + return None + + return catalog, skills + + +def discover_skills( + sources: list[SkillSource], afm_file_dir: Path +) -> dict[str, SkillInfo]: + """Discover skills from all sources, resolving paths relative to the AFM file directory.""" + skills: dict[str, SkillInfo] = {} + + for source in sources: + resolved_path = (afm_file_dir / source.path).resolve() + local_skills = discover_local_skills(resolved_path) + for name, info in local_skills.items(): + if name in skills: + logger.warning( + "Skill '%s' already discovered, skipping duplicate from %s", + name, + source.path, + ) + continue + skills[name] = info + + return skills + + +def discover_local_skills(path: Path) -> dict[str, SkillInfo]: + """Discover skills at a local path. + + If the path itself contains a SKILL.md, treat it as a single skill. + Otherwise, scan subdirectories for SKILL.md files. + """ + # Check if this path itself is a skill + skill_md_path = path / SKILL_FILE + if skill_md_path.is_file(): + info = parse_skill_md(skill_md_path, path) + return {info.name: info} + + # Scan subdirectories + skills: dict[str, SkillInfo] = {} + if not path.is_dir(): + return skills + + for entry in sorted(path.iterdir()): + if not entry.is_dir(): + continue + sub_skill_md = entry / SKILL_FILE + if not sub_skill_md.is_file(): + continue + + try: + info = parse_skill_md(sub_skill_md, entry) + except ValueError as e: + logger.error("Failed to parse skill at %s: %s", entry, e) + continue + + if info.name in skills: + logger.warning( + "Skill '%s' already discovered, skipping duplicate at %s", + info.name, + entry, + ) + continue + skills[info.name] = info + + return skills + + +def parse_skill_md(skill_md_path: Path, base_path: Path) -> SkillInfo: + """Parse a SKILL.md file and return a SkillInfo.""" + content = skill_md_path.read_text(encoding="utf-8") + resources = list_local_resources(base_path) + return parse_skill_md_content(content, base_path, resources) + + +def parse_skill_md_content( + content: str, base_path: Path, resources: list[str] +) -> SkillInfo: + """Parse SKILL.md content string into a SkillInfo. + + Expects YAML frontmatter with 'name' and 'description' fields. + """ + frontmatter, body = extract_raw_frontmatter(content) + if frontmatter is None: + raise ValueError("SKILL.md must start with YAML frontmatter (---)") + + name = frontmatter.get("name", "") + description = frontmatter.get("description", "") + + if not isinstance(name, str) or not name.strip(): + raise ValueError("SKILL.md 'name' field is required and must not be empty") + if not isinstance(description, str) or not description.strip(): + raise ValueError( + "SKILL.md 'description' field is required and must not be empty" + ) + + return SkillInfo( + name=name.strip(), + description=description.strip(), + body=body.strip(), + base_path=base_path, + resources=resources, + ) + + +def list_local_resources(base_path: Path) -> list[str]: + """List resource files in the references/ and assets/ directories of a skill.""" + resources: list[str] = [] + for dir_name in [REFERENCES_DIR, ASSETS_DIR]: + dir_path = base_path / dir_name + if not dir_path.is_dir(): + continue + for entry in sorted(dir_path.iterdir()): + if entry.is_file(): + resources.append(f"{dir_name}/{entry.name}") + return resources + + +def build_skill_catalog(skills: dict[str, SkillInfo]) -> str | None: + """Build a skill catalog string for inclusion in the system prompt.""" + if not skills: + return None + + skill_entries = "\n".join( + f" \n" + f" {xml_escape(info.name)}\n" + f" {xml_escape(info.description)}\n" + f" " + for info in skills.values() + ) + + return ( + "\n## Available Skills\n" + "\n" + "The following skills provide specialized instructions for specific tasks.\n" + "When a task matches a skill's description, call the activate_skill tool\n" + "with the skill's name to load its full instructions.\n" + "\n" + "\n" + f"{skill_entries}\n" + "\n" + ) + + +def activate_skill(name: str, skills: dict[str, SkillInfo]) -> str: + """Activate a skill by name and return its full instructions.""" + if name not in skills: + available = ", ".join(skills.keys()) + raise ValueError( + f"Skill '{name}' not found. Available skills: {available}" + ) + + info = skills[name] + resources_section = "" + if info.resources: + resource_entries = "\n".join( + f"{res}" for res in info.resources + ) + resources_section = ( + f"\n\n{resource_entries}\n\n" + "Use the read_skill_resource tool to read any of these files if needed.\n" + ) + + return ( + f'\n' + f"{info.body}\n" + f"{resources_section}" + "\n" + ) + + +def read_skill_resource( + skill_name: str, resource_path: str, skills: dict[str, SkillInfo] +) -> str: + """Read a resource file from a skill's references/ or assets/ directory.""" + if skill_name not in skills: + raise ValueError(f"Skill '{skill_name}' not found") + + # Validate path structure + parts = Path(resource_path).parts + if len(parts) < 2 or parts[0] not in (REFERENCES_DIR, ASSETS_DIR): + raise ValueError( + f"Resource path must start with '{REFERENCES_DIR}/' or '{ASSETS_DIR}/'" + ) + + if ".." in Path(resource_path).parts: + raise ValueError("Path traversal is not allowed in resource paths") + + info = skills[skill_name] + if resource_path not in info.resources: + available = ", ".join(info.resources) + raise ValueError( + f"Resource '{resource_path}' not found in skill '{skill_name}'. " + f"Available: {available}" + ) + + full_path = info.base_path / resource_path + return full_path.read_text(encoding="utf-8") + diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/empty_dir/.gitkeep b/python-interpreter/packages/afm-core/tests/fixtures/skills/empty_dir/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/invalid_skill/SKILL.md b/python-interpreter/packages/afm-core/tests/fixtures/skills/invalid_skill/SKILL.md new file mode 100644 index 0000000..b887386 --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/invalid_skill/SKILL.md @@ -0,0 +1,6 @@ +--- +name: "" +description: This skill has an empty name +--- + +This skill should fail to parse because the name is empty. diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/pr_summary/SKILL.md b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/pr_summary/SKILL.md new file mode 100644 index 0000000..b995c13 --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/pr_summary/SKILL.md @@ -0,0 +1,12 @@ +--- +name: pr-summary +description: Summarize pull requests into concise changelog entries +--- + +Summarize each merged pull request into a single changelog line. + +When a PR is merged, produce a one-line changelog entry in the format: + +- ``: (#) + +Where type is one of: feat, fix, refactor, docs, test, chore. diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/SKILL.md b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/SKILL.md new file mode 100644 index 0000000..569e4be --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/SKILL.md @@ -0,0 +1,15 @@ +--- +name: security-review +description: Review code for security vulnerabilities using OWASP guidelines +--- + +Perform a security-focused code review using OWASP guidelines. + +When reviewing code, check for: + +1. Injection flaws (SQL, command, path traversal) +2. Broken authentication or session management +3. Sensitive data exposure in logs or responses + +Consult the reference document for the full OWASP checklist. +Use the response template in assets/ to structure your findings. diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/assets/template.json b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/assets/template.json new file mode 100644 index 0000000..c55aa34 --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/assets/template.json @@ -0,0 +1 @@ +{"template": "security-review", "version": 1, "sections": ["summary", "findings", "severity", "remediation"]} diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/references/REFERENCE.md b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/references/REFERENCE.md new file mode 100644 index 0000000..e2f3b02 --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/references/REFERENCE.md @@ -0,0 +1,11 @@ +# OWASP Top 10 Quick Reference + +This is a reference file for security-review. + +| # | Category | +|---|---| +| A01 | Broken Access Control | +| A02 | Cryptographic Failures | +| A03 | Injection | +| A04 | Insecure Design | +| A05 | Security Misconfiguration | diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/no_frontmatter/SKILL.md b/python-interpreter/packages/afm-core/tests/fixtures/skills/no_frontmatter/SKILL.md new file mode 100644 index 0000000..89c1e46 --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/no_frontmatter/SKILL.md @@ -0,0 +1 @@ +This SKILL.md has no frontmatter at all. diff --git a/python-interpreter/packages/afm-core/tests/fixtures/skills/single_skill/SKILL.md b/python-interpreter/packages/afm-core/tests/fixtures/skills/single_skill/SKILL.md new file mode 100644 index 0000000..9a700da --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/fixtures/skills/single_skill/SKILL.md @@ -0,0 +1,12 @@ +--- +name: test-gen +description: Generate and run unit tests for code changes +--- + +When asked to test code, follow these steps: + +1. Identify the functions or modules that changed. +2. Write unit tests covering the happy path and key edge cases. +3. Run the tests and report results. + +Keep tests focused — one assertion per behavior. This is the body of the test skill. diff --git a/python-interpreter/packages/afm-core/tests/test_skills.py b/python-interpreter/packages/afm-core/tests/test_skills.py new file mode 100644 index 0000000..3821cef --- /dev/null +++ b/python-interpreter/packages/afm-core/tests/test_skills.py @@ -0,0 +1,329 @@ +# Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +# +# WSO2 LLC. licenses this file to you under the Apache License, +# Version 2.0 (the "License"); you may not use this file except +# in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from pathlib import Path + +import pytest + +from afm.models import AgentMetadata, LocalSkillSource, SkillInfo +from afm.skills import ( + activate_skill, + build_skill_catalog, + discover_local_skills, + discover_skills, + extract_skill_catalog, + list_local_resources, + parse_skill_md, + parse_skill_md_content, + read_skill_resource, +) + +FIXTURES_DIR = Path(__file__).parent / "fixtures" / "skills" + + +# --------------------------------------------------------------------------- +# parse_skill_md_content +# --------------------------------------------------------------------------- + + +class TestParseSkillMdContent: + def test_valid_skill(self) -> None: + content = "---\nname: my-skill\ndescription: Does things\n---\nBody here." + info = parse_skill_md_content(content, Path("/tmp"), []) + assert info.name == "my-skill" + assert info.description == "Does things" + assert info.body == "Body here." + assert info.resources == [] + + def test_strips_whitespace(self) -> None: + content = "---\nname: ' spaced '\ndescription: ' desc '\n---\n\n body \n" + info = parse_skill_md_content(content, Path("/tmp"), []) + assert info.name == "spaced" + assert info.description == "desc" + assert info.body == "body" + + def test_empty_name_raises(self) -> None: + content = "---\nname: ''\ndescription: Valid\n---\nBody." + with pytest.raises(ValueError, match="name"): + parse_skill_md_content(content, Path("/tmp"), []) + + def test_empty_description_raises(self) -> None: + content = "---\nname: valid\ndescription: ''\n---\nBody." + with pytest.raises(ValueError, match="description"): + parse_skill_md_content(content, Path("/tmp"), []) + + def test_missing_name_raises(self) -> None: + content = "---\ndescription: Valid\n---\nBody." + with pytest.raises(ValueError, match="name"): + parse_skill_md_content(content, Path("/tmp"), []) + + def test_missing_frontmatter_raises(self) -> None: + content = "No frontmatter here." + with pytest.raises(ValueError, match="frontmatter"): + parse_skill_md_content(content, Path("/tmp"), []) + + def test_unclosed_frontmatter_raises(self) -> None: + content = "---\nname: x\ndescription: y\nBody." + with pytest.raises(ValueError, match="Unclosed"): + parse_skill_md_content(content, Path("/tmp"), []) + + def test_preserves_resources(self) -> None: + content = "---\nname: s\ndescription: d\n---\nBody." + resources = ["references/REF.md", "assets/tpl.json"] + info = parse_skill_md_content(content, Path("/tmp"), resources) + assert info.resources == resources + + +# --------------------------------------------------------------------------- +# parse_skill_md (from file) +# --------------------------------------------------------------------------- + + +class TestParseSkillMd: + def test_single_skill(self) -> None: + skill_path = FIXTURES_DIR / "single_skill" / "SKILL.md" + info = parse_skill_md(skill_path, FIXTURES_DIR / "single_skill") + assert info.name == "test-gen" + assert "unit tests" in info.description + assert "Identify the functions" in info.body + + def test_invalid_skill_raises(self) -> None: + skill_path = FIXTURES_DIR / "invalid_skill" / "SKILL.md" + with pytest.raises(ValueError, match="name"): + parse_skill_md(skill_path, FIXTURES_DIR / "invalid_skill") + + def test_no_frontmatter_raises(self) -> None: + skill_path = FIXTURES_DIR / "no_frontmatter" / "SKILL.md" + with pytest.raises(ValueError, match="frontmatter"): + parse_skill_md(skill_path, FIXTURES_DIR / "no_frontmatter") + + +# --------------------------------------------------------------------------- +# list_local_resources +# --------------------------------------------------------------------------- + + +class TestListLocalResources: + def test_skill_with_resources(self) -> None: + base = FIXTURES_DIR / "multi_skills" / "security_review" + resources = list_local_resources(base) + assert "assets/template.json" in resources + assert "references/REFERENCE.md" in resources + + def test_skill_without_resources(self) -> None: + base = FIXTURES_DIR / "single_skill" + resources = list_local_resources(base) + assert resources == [] + + def test_nonexistent_path(self) -> None: + resources = list_local_resources(Path("/nonexistent/path")) + assert resources == [] + + +# --------------------------------------------------------------------------- +# discover_local_skills +# --------------------------------------------------------------------------- + + +class TestDiscoverLocalSkills: + def test_single_skill_directory(self) -> None: + skills = discover_local_skills(FIXTURES_DIR / "single_skill") + assert "test-gen" in skills + assert len(skills) == 1 + + def test_multi_skills_directory(self) -> None: + skills = discover_local_skills(FIXTURES_DIR / "multi_skills") + assert "pr-summary" in skills + assert "security-review" in skills + assert len(skills) == 2 + + def test_security_review_has_resources(self) -> None: + skills = discover_local_skills(FIXTURES_DIR / "multi_skills") + sr = skills["security-review"] + assert "references/REFERENCE.md" in sr.resources + assert "assets/template.json" in sr.resources + + def test_empty_directory(self) -> None: + skills = discover_local_skills(FIXTURES_DIR / "empty_dir") + assert skills == {} + + def test_invalid_skill_skipped(self) -> None: + # invalid_skill/ has a SKILL.md with empty name — treated as a single + # skill dir, so it raises (not skipped) when called directly + with pytest.raises(ValueError): + discover_local_skills(FIXTURES_DIR / "invalid_skill") + + def test_nonexistent_path(self) -> None: + skills = discover_local_skills(Path("/nonexistent/path")) + assert skills == {} + + +# --------------------------------------------------------------------------- +# discover_skills (multiple sources) +# --------------------------------------------------------------------------- + + +class TestDiscoverSkills: + def test_multiple_sources(self) -> None: + sources = [ + LocalSkillSource(path="single_skill"), + LocalSkillSource(path="multi_skills"), + ] + skills = discover_skills(sources, FIXTURES_DIR) + assert "test-gen" in skills + assert "pr-summary" in skills + assert "security-review" in skills + assert len(skills) == 3 + + def test_duplicate_across_sources_keeps_first(self) -> None: + """When two sources provide the same skill name, the first one wins.""" + sources = [ + LocalSkillSource(path="single_skill"), + LocalSkillSource(path="single_skill"), + ] + skills = discover_skills(sources, FIXTURES_DIR) + assert len(skills) == 1 + assert "test-gen" in skills + + def test_empty_sources(self) -> None: + skills = discover_skills([], FIXTURES_DIR) + assert skills == {} + + +# --------------------------------------------------------------------------- +# build_skill_catalog +# --------------------------------------------------------------------------- + + +class TestBuildSkillCatalog: + def test_builds_catalog(self) -> None: + skills = discover_local_skills(FIXTURES_DIR / "multi_skills") + catalog = build_skill_catalog(skills) + assert catalog is not None + assert "Available Skills" in catalog + assert "pr-summary" in catalog + assert "security-review" in catalog + assert "activate_skill" in catalog + + def test_empty_skills_returns_none(self) -> None: + assert build_skill_catalog({}) is None + + +# --------------------------------------------------------------------------- +# extract_skill_catalog +# --------------------------------------------------------------------------- + + +class TestExtractSkillCatalog: + def test_with_skills(self) -> None: + metadata = AgentMetadata( + skills=[LocalSkillSource(path="multi_skills")] + ) + result = extract_skill_catalog(metadata, FIXTURES_DIR) + assert result is not None + catalog, skills = result + assert "pr-summary" in skills + assert "security-review" in skills + assert "Available Skills" in catalog + + def test_no_skills_metadata(self) -> None: + metadata = AgentMetadata() + assert extract_skill_catalog(metadata, FIXTURES_DIR) is None + + def test_empty_skills_list(self) -> None: + metadata = AgentMetadata(skills=[]) + assert extract_skill_catalog(metadata, FIXTURES_DIR) is None + + +# --------------------------------------------------------------------------- +# activate_skill +# --------------------------------------------------------------------------- + + +class TestActivateSkill: + @pytest.fixture + def skills(self) -> dict[str, SkillInfo]: + return discover_local_skills(FIXTURES_DIR / "multi_skills") + + def test_activate_existing_skill(self, skills: dict[str, SkillInfo]) -> None: + result = activate_skill("pr-summary", skills) + assert '' in result + assert "changelog" in result + + def test_activate_skill_with_resources( + self, skills: dict[str, SkillInfo] + ) -> None: + result = activate_skill("security-review", skills) + assert "" in result + assert "references/REFERENCE.md" in result + assert "assets/template.json" in result + assert "read_skill_resource" in result + + def test_activate_skill_without_resources(self) -> None: + skills = discover_local_skills(FIXTURES_DIR / "single_skill") + result = activate_skill("test-gen", skills) + assert "" not in result + + def test_activate_unknown_skill_raises( + self, skills: dict[str, SkillInfo] + ) -> None: + with pytest.raises(ValueError, match="not found"): + activate_skill("nonexistent", skills) + + +# --------------------------------------------------------------------------- +# read_skill_resource +# --------------------------------------------------------------------------- + + +class TestReadSkillResource: + @pytest.fixture + def skills(self) -> dict[str, SkillInfo]: + return discover_local_skills(FIXTURES_DIR / "multi_skills") + + def test_read_reference(self, skills: dict[str, SkillInfo]) -> None: + content = read_skill_resource( + "security-review", "references/REFERENCE.md", skills + ) + assert "OWASP" in content + + def test_read_asset(self, skills: dict[str, SkillInfo]) -> None: + content = read_skill_resource( + "security-review", "assets/template.json", skills + ) + assert "security-review" in content + + def test_unknown_skill_raises(self, skills: dict[str, SkillInfo]) -> None: + with pytest.raises(ValueError, match="not found"): + read_skill_resource("nope", "references/REFERENCE.md", skills) + + def test_unlisted_resource_raises( + self, skills: dict[str, SkillInfo] + ) -> None: + with pytest.raises(ValueError, match="not found in skill"): + read_skill_resource( + "security-review", "references/SECRET.md", skills + ) + + def test_path_traversal_raises(self, skills: dict[str, SkillInfo]) -> None: + with pytest.raises(ValueError, match="traversal"): + read_skill_resource( + "security-review", "references/../../../etc/passwd", skills + ) + + def test_invalid_prefix_raises(self, skills: dict[str, SkillInfo]) -> None: + with pytest.raises(ValueError, match="must start with"): + read_skill_resource("security-review", "other/file.txt", skills) diff --git a/python-interpreter/packages/afm-langchain/src/afm_langchain/backend.py b/python-interpreter/packages/afm-langchain/src/afm_langchain/backend.py index 0808e39..eeac85c 100644 --- a/python-interpreter/packages/afm-langchain/src/afm_langchain/backend.py +++ b/python-interpreter/packages/afm-langchain/src/afm_langchain/backend.py @@ -36,9 +36,12 @@ from langchain_core.messages import AIMessage, HumanMessage, SystemMessage, ToolMessage from langchain_core.tools import BaseTool +from afm.skills import extract_skill_catalog + from .logging_utils import MCPStdioNoiseFilter from .providers import create_model_provider from .tools.mcp import MCPManager +from .tools.skills import ActivateSkillTool, ReadSkillResourceTool logger = logging.getLogger(__name__) @@ -64,6 +67,19 @@ def __init__( self._mcp_tools: list[BaseTool] = [] self._connected = False + # Skills + self._skill_catalog: str | None = None + self._skill_tools: list[BaseTool] = [] + if afm.source_dir and afm.metadata.skills: + result = extract_skill_catalog(afm.metadata, afm.source_dir) + if result is not None: + catalog, skills = result + self._skill_catalog = catalog + self._skill_tools = [ + ActivateSkillTool(skills=skills), + ReadSkillResourceTool(skills=skills), + ] + # Cache the active interface for signature validation self._interface = self._get_primary_interface() self._signature = self._get_signature() @@ -111,8 +127,7 @@ async def disconnect(self) -> None: logger.info("Disconnected from MCP servers") def _get_all_tools(self) -> list[BaseTool]: - - return self._external_tools + self._mcp_tools + return self._external_tools + self._mcp_tools + self._skill_tools @property def afm(self) -> AFMRecord: @@ -128,13 +143,16 @@ def description(self) -> str | None: @property def system_prompt(self) -> str: - return f"""# Role + prompt = f"""# Role {self._afm.role} # Instructions {self._afm.instructions}""" + if self._skill_catalog: + prompt += f"\n\n{self._skill_catalog}" + return prompt @property def max_iterations(self) -> int | None: diff --git a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py new file mode 100644 index 0000000..2808bc1 --- /dev/null +++ b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py @@ -0,0 +1,88 @@ +# Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). +# +# WSO2 LLC. licenses this file to you under the Apache License, +# Version 2.0 (the "License"); you may not use this file except +# in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from typing import Any, Type + +from afm.models import SkillInfo +from afm.skills import activate_skill, read_skill_resource +from langchain_core.tools import BaseTool +from pydantic import BaseModel, Field + + +class ActivateSkillInput(BaseModel): + name: str = Field( + description="The name of the skill to activate (must match a name from the available skills catalog)" + ) + + +class ReadSkillResourceInput(BaseModel): + skill_name: str = Field( + description="The name of the skill that owns the resource" + ) + resource_path: str = Field( + description="Relative path to the resource file (e.g., 'references/REFERENCE.md' or 'assets/template.json')" + ) + + +class ActivateSkillTool(BaseTool): + """Activates a skill by name and returns its full instructions.""" + + name: str = "activate_skill" + description: str = ( + "Activates a skill by name and returns its full instructions " + "along with available resources. Call this when a task matches " + "one of the available skills' descriptions." + ) + args_schema: Type[BaseModel] = ActivateSkillInput + + skills: dict[str, SkillInfo] + + def _run(self, name: str, **kwargs: Any) -> str: + try: + return activate_skill(name, self.skills) + except ValueError as e: + return f"Error: {e}" + + async def _arun(self, name: str, **kwargs: Any) -> str: + return self._run(name) + + +class ReadSkillResourceTool(BaseTool): + """Reads a resource file from a skill's references/ or assets/ directory.""" + + name: str = "read_skill_resource" + description: str = ( + "Reads a resource file from a skill's references/ or assets/ directory. " + "Only files listed in skill_resources after activating a skill can be read." + ) + args_schema: Type[BaseModel] = ReadSkillResourceInput + + skills: dict[str, SkillInfo] + + def _run( + self, skill_name: str, resource_path: str, **kwargs: Any + ) -> str: + try: + return read_skill_resource(skill_name, resource_path, self.skills) + except ValueError as e: + return f"Error: {e}" + + async def _arun( + self, skill_name: str, resource_path: str, **kwargs: Any + ) -> str: + return self._run(skill_name, resource_path) diff --git a/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py b/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py new file mode 100644 index 0000000..2b62b2a --- /dev/null +++ b/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py @@ -0,0 +1,169 @@ +# Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +# +# WSO2 LLC. licenses this file to you under the Apache License, +# Version 2.0 (the "License"); you may not use this file except +# in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from afm.models import AFMRecord, AgentMetadata, LocalSkillSource, SkillInfo +from afm.skills import discover_local_skills +from afm_langchain.backend import LangChainRunner +from afm_langchain.tools.skills import ActivateSkillTool, ReadSkillResourceTool + +# Reuse the fixtures from afm-core +FIXTURES_DIR = ( + Path(__file__).parent.parent.parent + / "afm-core" + / "tests" + / "fixtures" + / "skills" +) + + +@pytest.fixture +def multi_skills() -> dict[str, SkillInfo]: + return discover_local_skills(FIXTURES_DIR / "multi_skills") + + +@pytest.fixture +def mock_chat_model() -> MagicMock: + from unittest.mock import AsyncMock + + from langchain_core.messages import AIMessage + + model = MagicMock() + model.ainvoke = AsyncMock( + return_value=AIMessage(content="Hello!") + ) + return model + + +# --------------------------------------------------------------------------- +# ActivateSkillTool +# --------------------------------------------------------------------------- + + +class TestActivateSkillTool: + def test_activate_existing(self, multi_skills: dict[str, SkillInfo]) -> None: + tool = ActivateSkillTool(skills=multi_skills) + result = tool._run(name="pr-summary") + assert "changelog" in result + assert '' in result + + def test_activate_unknown_returns_error( + self, multi_skills: dict[str, SkillInfo] + ) -> None: + tool = ActivateSkillTool(skills=multi_skills) + result = tool._run(name="nonexistent") + assert "Error" in result + assert "not found" in result + + @pytest.mark.asyncio + async def test_arun(self, multi_skills: dict[str, SkillInfo]) -> None: + tool = ActivateSkillTool(skills=multi_skills) + result = await tool._arun(name="security-review") + assert "OWASP" in result + + +# --------------------------------------------------------------------------- +# ReadSkillResourceTool +# --------------------------------------------------------------------------- + + +class TestReadSkillResourceTool: + def test_read_valid_resource( + self, multi_skills: dict[str, SkillInfo] + ) -> None: + tool = ReadSkillResourceTool(skills=multi_skills) + result = tool._run( + skill_name="security-review", + resource_path="references/REFERENCE.md", + ) + assert "OWASP" in result + + def test_read_invalid_returns_error( + self, multi_skills: dict[str, SkillInfo] + ) -> None: + tool = ReadSkillResourceTool(skills=multi_skills) + result = tool._run( + skill_name="security-review", + resource_path="references/../../../etc/passwd", + ) + assert "Error" in result + assert "traversal" in result + + +# --------------------------------------------------------------------------- +# LangChainRunner integration +# --------------------------------------------------------------------------- + + +class TestLangChainRunnerSkills: + def test_skills_in_system_prompt(self, mock_chat_model: MagicMock) -> None: + afm = AFMRecord( + metadata=AgentMetadata( + name="Skill Agent", + skills=[LocalSkillSource(path="multi_skills")], + ), + role="You are a helpful assistant.", + instructions="Help the user.", + source_dir=FIXTURES_DIR, + ) + runner = LangChainRunner(afm, model=mock_chat_model) + assert "Available Skills" in runner.system_prompt + assert "pr-summary" in runner.system_prompt + assert "security-review" in runner.system_prompt + + def test_skill_tools_registered(self, mock_chat_model: MagicMock) -> None: + afm = AFMRecord( + metadata=AgentMetadata( + name="Skill Agent", + skills=[LocalSkillSource(path="multi_skills")], + ), + role="Test", + instructions="Test", + source_dir=FIXTURES_DIR, + ) + runner = LangChainRunner(afm, model=mock_chat_model) + tool_names = [t.name for t in runner.tools] + assert "activate_skill" in tool_names + assert "read_skill_resource" in tool_names + + def test_no_skills_no_catalog(self, mock_chat_model: MagicMock) -> None: + afm = AFMRecord( + metadata=AgentMetadata(name="No Skills Agent"), + role="Test", + instructions="Test", + ) + runner = LangChainRunner(afm, model=mock_chat_model) + assert "Available Skills" not in runner.system_prompt + assert runner.tools == [] + + def test_no_source_dir_skips_skills( + self, mock_chat_model: MagicMock + ) -> None: + afm = AFMRecord( + metadata=AgentMetadata( + skills=[LocalSkillSource(path="multi_skills")], + ), + role="Test", + instructions="Test", + # source_dir not set — skills should be skipped + ) + runner = LangChainRunner(afm, model=mock_chat_model) + assert "Available Skills" not in runner.system_prompt + assert runner.tools == [] From 3c7768755c8b0b3e68eb2d494b2bf18acb3589af Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 16 Mar 2026 10:01:53 +0530 Subject: [PATCH 11/16] Fix up skill path validation --- python-interpreter/packages/afm-core/src/afm/skills.py | 10 ++++++++++ .../packages/afm-core/tests/test_skills.py | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py index 23c2a33..3ed649c 100644 --- a/python-interpreter/packages/afm-core/src/afm/skills.py +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -61,8 +61,18 @@ def discover_skills( """Discover skills from all sources, resolving paths relative to the AFM file directory.""" skills: dict[str, SkillInfo] = {} + normalized_afm_dir = afm_file_dir.resolve() + for source in sources: + if Path(source.path).is_absolute(): + raise ValueError( + f"Skill source path must be relative, but got: {source.path}" + ) resolved_path = (afm_file_dir / source.path).resolve() + if not str(resolved_path).startswith(str(normalized_afm_dir)): + raise ValueError( + f"Skill source path '{source.path}' resolves outside the AFM file directory" + ) local_skills = discover_local_skills(resolved_path) for name, info in local_skills.items(): if name in skills: diff --git a/python-interpreter/packages/afm-core/tests/test_skills.py b/python-interpreter/packages/afm-core/tests/test_skills.py index 3821cef..f204e5f 100644 --- a/python-interpreter/packages/afm-core/tests/test_skills.py +++ b/python-interpreter/packages/afm-core/tests/test_skills.py @@ -202,6 +202,16 @@ def test_empty_sources(self) -> None: skills = discover_skills([], FIXTURES_DIR) assert skills == {} + def test_absolute_path_raises(self) -> None: + sources = [LocalSkillSource(path="/absolute/path/to/skills")] + with pytest.raises(ValueError, match="must be relative"): + discover_skills(sources, FIXTURES_DIR) + + def test_path_outside_afm_dir_raises(self) -> None: + sources = [LocalSkillSource(path="../../outside")] + with pytest.raises(ValueError, match="resolves outside"): + discover_skills(sources, FIXTURES_DIR) + # --------------------------------------------------------------------------- # build_skill_catalog From 8d524ecb9951fab5e55c9524e65b876f07ba3197 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 16 Mar 2026 14:50:01 +0530 Subject: [PATCH 12/16] Refactor code --- .../packages/afm-core/src/afm/parser.py | 42 +++++++------------ .../packages/afm-core/src/afm/skills.py | 14 +++---- .../packages/afm-core/tests/test_skills.py | 14 +++++++ .../src/afm_langchain/tools/skills.py | 8 ---- .../tests/test_langchain_skills.py | 2 +- 5 files changed, 36 insertions(+), 44 deletions(-) diff --git a/python-interpreter/packages/afm-core/src/afm/parser.py b/python-interpreter/packages/afm-core/src/afm/parser.py index f5bfede..9c8a6ba 100644 --- a/python-interpreter/packages/afm-core/src/afm/parser.py +++ b/python-interpreter/packages/afm-core/src/afm/parser.py @@ -92,38 +92,24 @@ def parse_afm_file(file_path: str | Path, *, resolve_env: bool = True) -> AFMRec def _extract_frontmatter(lines: list[str]) -> tuple[AgentMetadata, int]: - if not lines or lines[0].strip() != FRONTMATTER_DELIMITER: - # No frontmatter - return empty metadata - return AgentMetadata(), 0 - - end_index = None - for i in range(1, len(lines)): - if lines[i].strip() == FRONTMATTER_DELIMITER: - end_index = i - break - - if end_index is None: - raise AFMParseError("Unclosed frontmatter - missing closing '---'") - - yaml_lines = lines[1:end_index] - yaml_content = "\n".join(yaml_lines) - - if not yaml_content.strip(): - return AgentMetadata(), end_index + 1 - + content = "\n".join(lines) try: - yaml_data = yaml.safe_load(yaml_content) - except yaml.YAMLError as e: - raise AFMParseError(f"Invalid YAML in frontmatter: {e}") + raw, body = extract_raw_frontmatter(content) + except ValueError as e: + raise AFMParseError(str(e)) - if yaml_data is None: - return AgentMetadata(), end_index + 1 + if raw is None: + return AgentMetadata(), 0 - if not isinstance(yaml_data, dict): - raise AFMParseError("Frontmatter must be a YAML mapping/object") + # Calculate the body start line index + # The frontmatter occupies: opening --- + yaml lines + closing --- + body_start = len(lines) - len(body.splitlines()) if body else len(lines) + + if not raw: + return AgentMetadata(), body_start try: - metadata = AgentMetadata.model_validate(yaml_data) + metadata = AgentMetadata.model_validate(raw) except ValidationError as e: errors = e.errors() if errors: @@ -133,7 +119,7 @@ def _extract_frontmatter(lines: list[str]) -> tuple[AgentMetadata, int]: raise AFMValidationError(msg, field=field) raise AFMValidationError(str(e)) - return metadata, end_index + 1 + return metadata, body_start def _extract_role_and_instructions( diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py index 3ed649c..d1b16e2 100644 --- a/python-interpreter/packages/afm-core/src/afm/skills.py +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -18,13 +18,10 @@ import logging from pathlib import Path -from xml.sax.saxutils import escape as xml_escape - -from .models import AgentMetadata, SkillInfo, SkillSource +from .models import AgentMetadata, LocalSkillSource, SkillInfo, SkillSource from .parser import extract_raw_frontmatter logger = logging.getLogger(__name__) - SKILL_FILE = "SKILL.md" REFERENCES_DIR = "references" ASSETS_DIR = "assets" @@ -64,12 +61,15 @@ def discover_skills( normalized_afm_dir = afm_file_dir.resolve() for source in sources: + if not isinstance(source, LocalSkillSource): + logger.warning("Unsupported skill source type: %s", type(source).__name__) + continue if Path(source.path).is_absolute(): raise ValueError( f"Skill source path must be relative, but got: {source.path}" ) resolved_path = (afm_file_dir / source.path).resolve() - if not str(resolved_path).startswith(str(normalized_afm_dir)): + if not resolved_path.is_relative_to(normalized_afm_dir): raise ValueError( f"Skill source path '{source.path}' resolves outside the AFM file directory" ) @@ -186,8 +186,8 @@ def build_skill_catalog(skills: dict[str, SkillInfo]) -> str | None: skill_entries = "\n".join( f" \n" - f" {xml_escape(info.name)}\n" - f" {xml_escape(info.description)}\n" + f" {info.name}\n" + f" {info.description}\n" f" " for info in skills.values() ) diff --git a/python-interpreter/packages/afm-core/tests/test_skills.py b/python-interpreter/packages/afm-core/tests/test_skills.py index f204e5f..fe34d36 100644 --- a/python-interpreter/packages/afm-core/tests/test_skills.py +++ b/python-interpreter/packages/afm-core/tests/test_skills.py @@ -212,6 +212,20 @@ def test_path_outside_afm_dir_raises(self) -> None: with pytest.raises(ValueError, match="resolves outside"): discover_skills(sources, FIXTURES_DIR) + def test_sibling_prefix_path_raises(self, tmp_path: Path) -> None: + """A sibling directory sharing a name prefix must not pass validation. + + E.g. if afm_dir is '/a/skills', a path resolving to '/a/skills-evil' + should be rejected even though it starts with the same string prefix. + """ + afm_dir = tmp_path / "project" + sibling = tmp_path / "project-evil" + afm_dir.mkdir() + sibling.mkdir() + sources = [LocalSkillSource(path="../project-evil")] + with pytest.raises(ValueError, match="resolves outside"): + discover_skills(sources, afm_dir) + # --------------------------------------------------------------------------- # build_skill_catalog diff --git a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py index 2808bc1..82aa2ed 100644 --- a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py +++ b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py @@ -58,9 +58,6 @@ def _run(self, name: str, **kwargs: Any) -> str: except ValueError as e: return f"Error: {e}" - async def _arun(self, name: str, **kwargs: Any) -> str: - return self._run(name) - class ReadSkillResourceTool(BaseTool): """Reads a resource file from a skill's references/ or assets/ directory.""" @@ -81,8 +78,3 @@ def _run( return read_skill_resource(skill_name, resource_path, self.skills) except ValueError as e: return f"Error: {e}" - - async def _arun( - self, skill_name: str, resource_path: str, **kwargs: Any - ) -> str: - return self._run(skill_name, resource_path) diff --git a/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py b/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py index 2b62b2a..dd93fac 100644 --- a/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py +++ b/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py @@ -75,7 +75,7 @@ def test_activate_unknown_returns_error( @pytest.mark.asyncio async def test_arun(self, multi_skills: dict[str, SkillInfo]) -> None: tool = ActivateSkillTool(skills=multi_skills) - result = await tool._arun(name="security-review") + result = await tool.ainvoke({"name": "security-review"}) assert "OWASP" in result From 541677a528bba2871221095465e5400b11480103 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 19 Mar 2026 10:29:51 +0530 Subject: [PATCH 13/16] Allow absolute paths for skill dirs --- .../packages/afm-core/src/afm/skills.py | 16 ++++------ .../packages/afm-core/tests/test_skills.py | 29 ++++--------------- 2 files changed, 11 insertions(+), 34 deletions(-) diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py index d1b16e2..05f7df8 100644 --- a/python-interpreter/packages/afm-core/src/afm/skills.py +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -58,21 +58,15 @@ def discover_skills( """Discover skills from all sources, resolving paths relative to the AFM file directory.""" skills: dict[str, SkillInfo] = {} - normalized_afm_dir = afm_file_dir.resolve() - for source in sources: if not isinstance(source, LocalSkillSource): logger.warning("Unsupported skill source type: %s", type(source).__name__) continue - if Path(source.path).is_absolute(): - raise ValueError( - f"Skill source path must be relative, but got: {source.path}" - ) - resolved_path = (afm_file_dir / source.path).resolve() - if not resolved_path.is_relative_to(normalized_afm_dir): - raise ValueError( - f"Skill source path '{source.path}' resolves outside the AFM file directory" - ) + source_path = Path(source.path) + resolved_path = ( + source_path if source_path.is_absolute() + else (afm_file_dir / source.path).resolve() + ) local_skills = discover_local_skills(resolved_path) for name, info in local_skills.items(): if name in skills: diff --git a/python-interpreter/packages/afm-core/tests/test_skills.py b/python-interpreter/packages/afm-core/tests/test_skills.py index fe34d36..cdc0807 100644 --- a/python-interpreter/packages/afm-core/tests/test_skills.py +++ b/python-interpreter/packages/afm-core/tests/test_skills.py @@ -202,29 +202,12 @@ def test_empty_sources(self) -> None: skills = discover_skills([], FIXTURES_DIR) assert skills == {} - def test_absolute_path_raises(self) -> None: - sources = [LocalSkillSource(path="/absolute/path/to/skills")] - with pytest.raises(ValueError, match="must be relative"): - discover_skills(sources, FIXTURES_DIR) - - def test_path_outside_afm_dir_raises(self) -> None: - sources = [LocalSkillSource(path="../../outside")] - with pytest.raises(ValueError, match="resolves outside"): - discover_skills(sources, FIXTURES_DIR) - - def test_sibling_prefix_path_raises(self, tmp_path: Path) -> None: - """A sibling directory sharing a name prefix must not pass validation. - - E.g. if afm_dir is '/a/skills', a path resolving to '/a/skills-evil' - should be rejected even though it starts with the same string prefix. - """ - afm_dir = tmp_path / "project" - sibling = tmp_path / "project-evil" - afm_dir.mkdir() - sibling.mkdir() - sources = [LocalSkillSource(path="../project-evil")] - with pytest.raises(ValueError, match="resolves outside"): - discover_skills(sources, afm_dir) + def test_absolute_path_accepted(self) -> None: + abs_path = str((FIXTURES_DIR / "single_skill").resolve()) + sources = [LocalSkillSource(path=abs_path)] + skills = discover_skills(sources, Path("/some/other/dir")) + assert len(skills) == 1 + assert "test-gen" in skills # --------------------------------------------------------------------------- From ee7e20172a48f9608ef1cca9fad4722da993f358 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 19 Mar 2026 12:05:26 +0530 Subject: [PATCH 14/16] Address suggestions --- .../packages/afm-core/src/afm/skills.py | 5 ++++- .../packages/afm-core/tests/test_skills.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py index 05f7df8..d11cb5c 100644 --- a/python-interpreter/packages/afm-core/src/afm/skills.py +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -251,6 +251,9 @@ def read_skill_resource( f"Available: {available}" ) - full_path = info.base_path / resource_path + full_path = (info.base_path / resource_path).resolve() + if not full_path.is_relative_to(info.base_path.resolve()): + raise ValueError("Path traversal is not allowed in resource paths") + return full_path.read_text(encoding="utf-8") diff --git a/python-interpreter/packages/afm-core/tests/test_skills.py b/python-interpreter/packages/afm-core/tests/test_skills.py index cdc0807..b80b54a 100644 --- a/python-interpreter/packages/afm-core/tests/test_skills.py +++ b/python-interpreter/packages/afm-core/tests/test_skills.py @@ -331,6 +331,24 @@ def test_path_traversal_raises(self, skills: dict[str, SkillInfo]) -> None: "security-review", "references/../../../etc/passwd", skills ) + def test_symlink_traversal_raises( + self, skills: dict[str, SkillInfo], tmp_path: Path + ) -> None: + # Create a symlink inside the skill's references/ dir pointing outside + skill_info = skills["security-review"] + outside_file = tmp_path / "secret.txt" + outside_file.write_text("secret data") + symlink = skill_info.base_path / "references" / "sneaky.md" + symlink.symlink_to(outside_file) + try: + # Add the symlink to resources so it passes the allowlist check + skill_info.resources.append("references/sneaky.md") + with pytest.raises(ValueError, match="traversal"): + read_skill_resource("security-review", "references/sneaky.md", skills) + finally: + symlink.unlink() + skill_info.resources.remove("references/sneaky.md") + def test_invalid_prefix_raises(self, skills: dict[str, SkillInfo]) -> None: with pytest.raises(ValueError, match="must start with"): read_skill_resource("security-review", "other/file.txt", skills) From eaa5ba09ae2595c22db5d97a3978a5c0c37a38f4 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 20 Mar 2026 09:51:37 +0530 Subject: [PATCH 15/16] Fix formatting --- .../packages/afm-core/src/afm/skills.py | 16 ++++--------- .../packages/afm-core/tests/test_skills.py | 24 +++++-------------- .../src/afm_langchain/tools/skills.py | 8 ++----- .../tests/test_langchain_skills.py | 18 ++++---------- 4 files changed, 17 insertions(+), 49 deletions(-) diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py index d11cb5c..54f7dd1 100644 --- a/python-interpreter/packages/afm-core/src/afm/skills.py +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -41,9 +41,7 @@ def extract_skill_catalog( if not skills: return None - logger.debug( - "Loaded %d skill(s): %s", len(skills), ", ".join(skills.keys()) - ) + logger.debug("Loaded %d skill(s): %s", len(skills), ", ".join(skills.keys())) catalog = build_skill_catalog(skills) if catalog is None: @@ -64,7 +62,8 @@ def discover_skills( continue source_path = Path(source.path) resolved_path = ( - source_path if source_path.is_absolute() + source_path + if source_path.is_absolute() else (afm_file_dir / source.path).resolve() ) local_skills = discover_local_skills(resolved_path) @@ -203,16 +202,12 @@ def activate_skill(name: str, skills: dict[str, SkillInfo]) -> str: """Activate a skill by name and return its full instructions.""" if name not in skills: available = ", ".join(skills.keys()) - raise ValueError( - f"Skill '{name}' not found. Available skills: {available}" - ) + raise ValueError(f"Skill '{name}' not found. Available skills: {available}") info = skills[name] resources_section = "" if info.resources: - resource_entries = "\n".join( - f"{res}" for res in info.resources - ) + resource_entries = "\n".join(f"{res}" for res in info.resources) resources_section = ( f"\n\n{resource_entries}\n\n" "Use the read_skill_resource tool to read any of these files if needed.\n" @@ -256,4 +251,3 @@ def read_skill_resource( raise ValueError("Path traversal is not allowed in resource paths") return full_path.read_text(encoding="utf-8") - diff --git a/python-interpreter/packages/afm-core/tests/test_skills.py b/python-interpreter/packages/afm-core/tests/test_skills.py index b80b54a..8c26e95 100644 --- a/python-interpreter/packages/afm-core/tests/test_skills.py +++ b/python-interpreter/packages/afm-core/tests/test_skills.py @@ -236,9 +236,7 @@ def test_empty_skills_returns_none(self) -> None: class TestExtractSkillCatalog: def test_with_skills(self) -> None: - metadata = AgentMetadata( - skills=[LocalSkillSource(path="multi_skills")] - ) + metadata = AgentMetadata(skills=[LocalSkillSource(path="multi_skills")]) result = extract_skill_catalog(metadata, FIXTURES_DIR) assert result is not None catalog, skills = result @@ -270,9 +268,7 @@ def test_activate_existing_skill(self, skills: dict[str, SkillInfo]) -> None: assert '' in result assert "changelog" in result - def test_activate_skill_with_resources( - self, skills: dict[str, SkillInfo] - ) -> None: + def test_activate_skill_with_resources(self, skills: dict[str, SkillInfo]) -> None: result = activate_skill("security-review", skills) assert "" in result assert "references/REFERENCE.md" in result @@ -284,9 +280,7 @@ def test_activate_skill_without_resources(self) -> None: result = activate_skill("test-gen", skills) assert "" not in result - def test_activate_unknown_skill_raises( - self, skills: dict[str, SkillInfo] - ) -> None: + def test_activate_unknown_skill_raises(self, skills: dict[str, SkillInfo]) -> None: with pytest.raises(ValueError, match="not found"): activate_skill("nonexistent", skills) @@ -308,22 +302,16 @@ def test_read_reference(self, skills: dict[str, SkillInfo]) -> None: assert "OWASP" in content def test_read_asset(self, skills: dict[str, SkillInfo]) -> None: - content = read_skill_resource( - "security-review", "assets/template.json", skills - ) + content = read_skill_resource("security-review", "assets/template.json", skills) assert "security-review" in content def test_unknown_skill_raises(self, skills: dict[str, SkillInfo]) -> None: with pytest.raises(ValueError, match="not found"): read_skill_resource("nope", "references/REFERENCE.md", skills) - def test_unlisted_resource_raises( - self, skills: dict[str, SkillInfo] - ) -> None: + def test_unlisted_resource_raises(self, skills: dict[str, SkillInfo]) -> None: with pytest.raises(ValueError, match="not found in skill"): - read_skill_resource( - "security-review", "references/SECRET.md", skills - ) + read_skill_resource("security-review", "references/SECRET.md", skills) def test_path_traversal_raises(self, skills: dict[str, SkillInfo]) -> None: with pytest.raises(ValueError, match="traversal"): diff --git a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py index 82aa2ed..c84239a 100644 --- a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py +++ b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py @@ -31,9 +31,7 @@ class ActivateSkillInput(BaseModel): class ReadSkillResourceInput(BaseModel): - skill_name: str = Field( - description="The name of the skill that owns the resource" - ) + skill_name: str = Field(description="The name of the skill that owns the resource") resource_path: str = Field( description="Relative path to the resource file (e.g., 'references/REFERENCE.md' or 'assets/template.json')" ) @@ -71,9 +69,7 @@ class ReadSkillResourceTool(BaseTool): skills: dict[str, SkillInfo] - def _run( - self, skill_name: str, resource_path: str, **kwargs: Any - ) -> str: + def _run(self, skill_name: str, resource_path: str, **kwargs: Any) -> str: try: return read_skill_resource(skill_name, resource_path, self.skills) except ValueError as e: diff --git a/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py b/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py index dd93fac..4cbfdf0 100644 --- a/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py +++ b/python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py @@ -26,11 +26,7 @@ # Reuse the fixtures from afm-core FIXTURES_DIR = ( - Path(__file__).parent.parent.parent - / "afm-core" - / "tests" - / "fixtures" - / "skills" + Path(__file__).parent.parent.parent / "afm-core" / "tests" / "fixtures" / "skills" ) @@ -46,9 +42,7 @@ def mock_chat_model() -> MagicMock: from langchain_core.messages import AIMessage model = MagicMock() - model.ainvoke = AsyncMock( - return_value=AIMessage(content="Hello!") - ) + model.ainvoke = AsyncMock(return_value=AIMessage(content="Hello!")) return model @@ -85,9 +79,7 @@ async def test_arun(self, multi_skills: dict[str, SkillInfo]) -> None: class TestReadSkillResourceTool: - def test_read_valid_resource( - self, multi_skills: dict[str, SkillInfo] - ) -> None: + def test_read_valid_resource(self, multi_skills: dict[str, SkillInfo]) -> None: tool = ReadSkillResourceTool(skills=multi_skills) result = tool._run( skill_name="security-review", @@ -153,9 +145,7 @@ def test_no_skills_no_catalog(self, mock_chat_model: MagicMock) -> None: assert "Available Skills" not in runner.system_prompt assert runner.tools == [] - def test_no_source_dir_skips_skills( - self, mock_chat_model: MagicMock - ) -> None: + def test_no_source_dir_skips_skills(self, mock_chat_model: MagicMock) -> None: afm = AFMRecord( metadata=AgentMetadata( skills=[LocalSkillSource(path="multi_skills")], From fed87efe8e463ecfc9a08acb9a4d42aead7193da Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 20 Mar 2026 11:02:10 +0530 Subject: [PATCH 16/16] Address review comments --- ballerina-interpreter/parser.bal | 3 ++- ballerina-interpreter/skills.bal | 12 ++++++++++-- .../packages/afm-core/src/afm/models.py | 4 ++-- .../packages/afm-core/src/afm/skills.py | 7 ++++++- .../afm-langchain/src/afm_langchain/tools/skills.py | 2 +- 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/ballerina-interpreter/parser.bal b/ballerina-interpreter/parser.bal index 8657ffb..8758272 100644 --- a/ballerina-interpreter/parser.bal +++ b/ballerina-interpreter/parser.bal @@ -22,7 +22,8 @@ function parseAfm(string content) returns AFMRecord|error { AgentMetadata? metadata = (); string body; - if resolvedContent.startsWith(FRONTMATTER_DELIMITER) { + string[] resolvedLines = splitLines(resolvedContent); + if resolvedLines.length() > 0 && resolvedLines[0].trim() == FRONTMATTER_DELIMITER { map frontmatterMap; [frontmatterMap, body] = check extractFrontMatter(resolvedContent); metadata = check frontmatterMap.fromJsonWithType(); diff --git a/ballerina-interpreter/skills.bal b/ballerina-interpreter/skills.bal index 2c84ffc..e7f0e10 100644 --- a/ballerina-interpreter/skills.bal +++ b/ballerina-interpreter/skills.bal @@ -73,7 +73,10 @@ function discoverLocalSkills(string path) returns map|error { map skills = {}; file:MetaData[] entries = check file:readDir(absPath); - foreach file:MetaData entry in entries { + file:MetaData[] sortedEntries = from file:MetaData entry in entries + order by entry.absPath ascending + select entry; + foreach file:MetaData entry in sortedEntries { if !entry.dir { continue; } @@ -235,7 +238,12 @@ ${resourcesSection} } string fullPath = check file:joinPath(info.basePath, resourcePath); - return io:fileReadString(fullPath); + string canonicalPath = check file:getAbsolutePath(fullPath); + string canonicalBase = check file:getAbsolutePath(info.basePath); + if !canonicalPath.startsWith(canonicalBase) { + return error("Path traversal is not allowed in resource paths"); + } + return io:fileReadString(canonicalPath); } public isolated function getTools() returns ai:ToolConfig[] => diff --git a/python-interpreter/packages/afm-core/src/afm/models.py b/python-interpreter/packages/afm-core/src/afm/models.py index 4772a98..101f6d9 100644 --- a/python-interpreter/packages/afm-core/src/afm/models.py +++ b/python-interpreter/packages/afm-core/src/afm/models.py @@ -213,7 +213,7 @@ class SkillInfo(BaseModel): name: str description: str body: str - base_path: Path + base_path: Path = Field(exclude=True) resources: list[str] = Field(default_factory=list) @@ -242,7 +242,7 @@ class AFMRecord(BaseModel): metadata: AgentMetadata role: str instructions: str - source_dir: Path | None = None + source_dir: Path | None = Field(default=None, exclude=True) class LiteralSegment(BaseModel): diff --git a/python-interpreter/packages/afm-core/src/afm/skills.py b/python-interpreter/packages/afm-core/src/afm/skills.py index 54f7dd1..bd586fc 100644 --- a/python-interpreter/packages/afm-core/src/afm/skills.py +++ b/python-interpreter/packages/afm-core/src/afm/skills.py @@ -250,4 +250,9 @@ def read_skill_resource( if not full_path.is_relative_to(info.base_path.resolve()): raise ValueError("Path traversal is not allowed in resource paths") - return full_path.read_text(encoding="utf-8") + try: + return full_path.read_text(encoding="utf-8") + except UnicodeDecodeError: + raise ValueError( + f"Resource '{resource_path}' is a binary file and cannot be read as text" + ) diff --git a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py index c84239a..0f21882 100644 --- a/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py +++ b/python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py @@ -72,5 +72,5 @@ class ReadSkillResourceTool(BaseTool): def _run(self, skill_name: str, resource_path: str, **kwargs: Any) -> str: try: return read_skill_resource(skill_name, resource_path, self.skills) - except ValueError as e: + except (ValueError, UnicodeDecodeError) as e: return f"Error: {e}"