Conversation
Support Agent Skills with the Ballerina Interpreter
Support Agent Skills with the Python Interpreter
Contributor
WalkthroughAdds local "skills" support across Ballerina and Python interpreters: new skill discovery/parsing, catalog generation, secure resource access, AFM metadata/extensions, parser frontmatter extraction, agent/toolkit integration, LangChain tools, tests/fixtures, and dependency/template updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant SkillsMgr as Skills Manager
participant FS as File System
participant Catalog as Skill Catalog
participant Toolkit as Skills Toolkit
Agent->>SkillsMgr: extractSkillCatalog(metadata, afmFileDir)
SkillsMgr->>FS: resolve skill source paths
SkillsMgr->>FS: read SKILL.md files
FS-->>SkillsMgr: frontmatter + body
SkillsMgr->>SkillsMgr: validate & collect SkillInfo (resources)
SkillsMgr->>Catalog: build catalog text (names/descriptions)
Catalog-->>Agent: catalog text
Agent->>Toolkit: activateSkill(skillName)
Toolkit->>Toolkit: validate skill existence
Toolkit->>FS: read referenced resource file
FS-->>Toolkit: file content
Toolkit-->>Agent: skill body (+ resources section or error)
sequenceDiagram
participant User as AFM File / User
participant Parser as Parser
participant FM as Frontmatter Handler
participant Validator as Validator
User->>Parser: parseAfm(content)
Parser->>FM: extractFrontMatter(content)
FM->>FM: detect '---' delimiters, parse YAML
FM-->>Parser: (metadata map, body)
Parser->>Validator: validateHttpVariables & skills paths
Validator-->>Parser: validation result
Parser-->>User: AFMRecord (metadata optional, skills optional)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
ballerina-interpreter/tests/agent_test.bal (1)
372-372: Use the AFM’s real parent directory in these test startups.Lines 372 and 399 hardcode
"tests", while production resolvesafmFileDirfrom the AFM file’s absolute parent path first. That keeps these tests coupled to the runner’s current working directory and stops them from covering the same relative-skill resolution behavior used inmain.bal.Also applies to: 399-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-interpreter/tests/agent_test.bal` at line 372, The tests call runAgentFromAFM(afm, testPort, "tests") with a hardcoded "tests" path which diverges from production behavior; change the third argument to use the AFM file's real parent directory (derive it from the afm file object, e.g., afm.parent() or afmFileDir resolved from afm.absolutePath) so the test uses the AFM’s actual parent folder for relative-skill resolution; update both occurrences (the call at runAgentFromAFM and the similar call around line 399) to pass the computed parent directory instead of the string "tests".ballerina-interpreter/tests/skills_test.bal (1)
582-586: StrengthengetTools()assertion to avoid false positives.Line 585 only checks count. This can pass even if wrong tools are returned. Assert expected tool names (e.g.,
activateSkill,readSkillResource) too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-interpreter/tests/skills_test.bal` around lines 582 - 586, The current test only checks tools.length() after calling toolkit.getTools(), which can yield false positives; modify the test that uses getTools() (variable tools) to also assert the returned tools include the expected names "activateSkill" and "readSkillResource" (e.g., by iterating tools and asserting each tool's name property or getName() value or by collecting names into a list/set and comparing to the expected set), rather than relying solely on tools.length().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ballerina-interpreter/parser.bal`:
- Around line 25-31: The current check uses
resolvedContent.startsWith(FRONTMATTER_DELIMITER) which incorrectly treats any
string beginning with '---' as frontmatter; change the guard to only treat
frontmatter when the very first line equals the delimiter. Concretely, extract
the first line from resolvedContent (up to the first '\n' or end-of-string) and
test that firstLine.trim() == FRONTMATTER_DELIMITER before calling
extractFrontMatter; keep the rest of the logic (frontmatterMap, body, metadata
assignments) unchanged and reference FRONTMATTER_DELIMITER, resolvedContent,
extractFrontMatter, metadata, and body when making the update.
In `@ballerina-interpreter/skills.bal`:
- Around line 127-145: listLocalResources currently trusts directory entries and
can follow symlinks outside the skill tree; canonicalize and validate paths
before adding/reading them by resolving both the directory and each entry to
their absolute/canonical forms (e.g., via file:resolvePath or equivalent) and
ensure the resolved entry path is contained within the resolved base directory
for that resource (use string startsWith or path comparison) before pushing into
resources; apply the same canonicalization+containment check in
readSkillResource so symlinks cannot escape the skill tree (refer to functions
listLocalResources and readSkillResource and the variables
dirPath/entry.absPath).
- Around line 217-238: readSkillResource currently only verifies the resource
exists but does not verify the skill was previously activated, allowing bypass
of progressive disclosure; after obtaining SkillInfo (the `info` variable) add a
guard that the skill is in the activated state (use the existing activation
tracking used by `activateSkill` — e.g. `info.active` or
`self.activeSkills`/`self.activatedSkills`) and return an error like "Skill not
activated" if it isn't; ensure the new check runs before returning the resource
and mirror the same activation-check semantics used by `activateSkill`.
In
`@ballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.md`:
- Around line 6-8: The discovery loop in skills.bal uses file:readDir() which
returns unsorted entries causing
testDiscoverLocalSkillsDuplicateSubdirectories() to be order-fragile; fix by
sorting the directory entries by their path/name before iterating (e.g., collect
entries into a list and call a sort by entry.path or entry.name) so the
first-occurrence tie-breaker is deterministic and the expected "alphabetically
first" skill (error_translator) always wins.
In `@python-interpreter/packages/afm-core/src/afm/models.py`:
- Around line 210-217: SkillInfo.base_path and AFMRecord.source_dir are
runtime-only filesystem paths that must not be serialized; update their
declarations to exclude them from model dumps/JSON. For both SkillInfo.base_path
and the AFMRecord.source_dir field (used/assigned by parse_afm_file()), mark the
fields as non-serializable (e.g., use Field(..., exclude=True) or convert them
to PrivateAttr if you want them to be non-model attributes) so that
model_dump()/json() won’t expose host filesystem paths; ensure tests and any
code referencing these attributes still access them at runtime but serialization
omits them.
In `@python-interpreter/packages/afm-core/src/afm/skills.py`:
- Around line 163-173: list_local_resources advertises every file under
REFERENCES_DIR and ASSETS_DIR including binaries that read_skill_resource
decodes as UTF-8 (which can raise UnicodeDecodeError) and the
ReadSkillResourceTool only catches ValueError; update read_skill_resource (and
any callers) to catch UnicodeDecodeError and re-raise as ValueError with a clear
message, and enforce a maximum file-size limit (e.g., reject/raise ValueError if
size > threshold) before returning contents so large assets aren’t served; also
consider updating ReadSkillResourceTool to rely on ValueError only so behavior
is consistent with list_local_resources' advertised files.
- Around line 176-199: The XML-style prompt blocks in build_skill_catalog are
injecting unescaped skill fields (SkillInfo.name, SkillInfo.description and any
resource names) directly into tags and attributes (<available_skills>, <skill>,
<name>, <description>, and the name="..." attribute used with activate_skill),
so update build_skill_catalog to escape or validate those values before
formatting (use a single util like xml_escape or html.escape to replace &, <, >,
" and '). Also apply the same escaping/validation to the other prompt-building
code that interpolates <file> elements and name="..." attributes so no
user/FS-provided string can break the wrapper or change prompt structure.
In `@python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py`:
- Around line 42-80: The tools currently allow reading any skill resource
because ActivateSkillTool does not record activations and ReadSkillResourceTool
uses the full skills map; add a shared per-runner set (e.g., activated_skills:
set[str]) accessible to both ActivateSkillTool and ReadSkillResourceTool, update
ActivateSkillTool._run to add the skill name into activated_skills after a
successful activate_skill(...) call, and modify ReadSkillResourceTool._run to
check the skill_name is present in activated_skills and raise/return an error if
not activated before calling read_skill_resource(...); reference the classes
ActivateSkillTool and ReadSkillResourceTool and functions activate_skill and
read_skill_resource when making the changes.
---
Nitpick comments:
In `@ballerina-interpreter/tests/agent_test.bal`:
- Line 372: The tests call runAgentFromAFM(afm, testPort, "tests") with a
hardcoded "tests" path which diverges from production behavior; change the third
argument to use the AFM file's real parent directory (derive it from the afm
file object, e.g., afm.parent() or afmFileDir resolved from afm.absolutePath) so
the test uses the AFM’s actual parent folder for relative-skill resolution;
update both occurrences (the call at runAgentFromAFM and the similar call around
line 399) to pass the computed parent directory instead of the string "tests".
In `@ballerina-interpreter/tests/skills_test.bal`:
- Around line 582-586: The current test only checks tools.length() after calling
toolkit.getTools(), which can yield false positives; modify the test that uses
getTools() (variable tools) to also assert the returned tools include the
expected names "activateSkill" and "readSkillResource" (e.g., by iterating tools
and asserting each tool's name property or getName() value or by collecting
names into a list/set and comparing to the expected set), rather than relying
solely on tools.length().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d5a48f3-67f9-44a3-9b80-29ffa1f1f456
📒 Files selected for processing (41)
ballerina-interpreter/Dependencies.tomlballerina-interpreter/README.mdballerina-interpreter/agent.balballerina-interpreter/main.balballerina-interpreter/parser.balballerina-interpreter/skills.balballerina-interpreter/tests/agent_test.balballerina-interpreter/tests/main_test.balballerina-interpreter/tests/skills/duplicate_names/error_translator/SKILL.mdballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.mdballerina-interpreter/tests/skills/empty_dir/.gitkeepballerina-interpreter/tests/skills/invalid_skill/SKILL.mdballerina-interpreter/tests/skills/multi_skills/pr_summary/SKILL.mdballerina-interpreter/tests/skills/multi_skills/security_review/SKILL.mdballerina-interpreter/tests/skills/multi_skills/security_review/assets/template.jsonballerina-interpreter/tests/skills/multi_skills/security_review/references/REFERENCE.mdballerina-interpreter/tests/skills/no_frontmatter/SKILL.mdballerina-interpreter/tests/skills/partial_invalid/invalid_skill/SKILL.mdballerina-interpreter/tests/skills/partial_invalid/valid_skill/SKILL.mdballerina-interpreter/tests/skills/single_skill/SKILL.mdballerina-interpreter/tests/skills/source1/release_notes/SKILL.mdballerina-interpreter/tests/skills/source2/release_notes/SKILL.mdballerina-interpreter/tests/skills_test.balballerina-interpreter/types.balballerina-interpreter/web_chat_ui.balpython-interpreter/packages/afm-core/src/afm/models.pypython-interpreter/packages/afm-core/src/afm/parser.pypython-interpreter/packages/afm-core/src/afm/skills.pypython-interpreter/packages/afm-core/tests/fixtures/skills/empty_dir/.gitkeeppython-interpreter/packages/afm-core/tests/fixtures/skills/invalid_skill/SKILL.mdpython-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/pr_summary/SKILL.mdpython-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/SKILL.mdpython-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/assets/template.jsonpython-interpreter/packages/afm-core/tests/fixtures/skills/multi_skills/security_review/references/REFERENCE.mdpython-interpreter/packages/afm-core/tests/fixtures/skills/no_frontmatter/SKILL.mdpython-interpreter/packages/afm-core/tests/fixtures/skills/single_skill/SKILL.mdpython-interpreter/packages/afm-core/tests/test_skills.pypython-interpreter/packages/afm-langchain/src/afm_langchain/backend.pypython-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.pypython-interpreter/packages/afm-langchain/tests/test_langchain_skills.pytemplates/agent_template.afm.md
Comment on lines
+127
to
+145
| 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(string `${dir}/${check file:basename(entry.absPath)}`); | ||
| } | ||
| } | ||
| } on fail error e { | ||
| log:printWarn(string `Failed to read directory ${dir}`, 'error = e); | ||
| } | ||
| } | ||
| return resources; |
Contributor
There was a problem hiding this comment.
Canonicalize resource paths before reading them.
Lines 127-145 whitelist every non-directory entry under references/ and assets/, and Lines 237-238 read that path after only a textual .. check. A symlink inside one of those directories can still point outside the skill tree and expose arbitrary host files through readSkillResource, which defeats the path-traversal guard.
Also applies to: 237-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-interpreter/skills.bal` around lines 127 - 145, listLocalResources
currently trusts directory entries and can follow symlinks outside the skill
tree; canonicalize and validate paths before adding/reading them by resolving
both the directory and each entry to their absolute/canonical forms (e.g., via
file:resolvePath or equivalent) and ensure the resolved entry path is contained
within the resolved base directory for that resource (use string startsWith or
path comparison) before pushing into resources; apply the same
canonicalization+containment check in readSkillResource so symlinks cannot
escape the skill tree (refer to functions listLocalResources and
readSkillResource and the variables dirPath/entry.absPath).
ballerina-interpreter/skills.bal
Outdated
Comment on lines
+217
to
+238
| isolated function readSkillResource(string skillName, string resourcePath) returns string|error { | ||
| if !self.skills.hasKey(skillName) { | ||
| return error(string `Skill '${skillName}' not found`); | ||
| } | ||
|
|
||
| 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}/'`); | ||
| } | ||
|
|
||
| if segments.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); |
Contributor
There was a problem hiding this comment.
readSkillResource can be called before activateSkill.
Line 232 only checks that the resource was discovered for the skill. There is no state tying readSkillResource to a prior activateSkill call, so a model can bypass the intended progressive-disclosure step by guessing a conventional filename like references/REFERENCE.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-interpreter/skills.bal` around lines 217 - 238, readSkillResource
currently only verifies the resource exists but does not verify the skill was
previously activated, allowing bypass of progressive disclosure; after obtaining
SkillInfo (the `info` variable) add a guard that the skill is in the activated
state (use the existing activation tracking used by `activateSkill` — e.g.
`info.active` or `self.activeSkills`/`self.activatedSkills`) and return an error
like "Skill not activated" if it isn't; ensure the new check runs before
returning the resource and mirror the same activation-check semantics used by
`activateSkill`.
ballerina-interpreter/tests/skills/duplicate_names/error_translator_dup/SKILL.md
Show resolved
Hide resolved
Comment on lines
+176
to
+199
| 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" <skill>\n" | ||
| f" <name>{info.name}</name>\n" | ||
| f" <description>{info.description}</description>\n" | ||
| f" </skill>" | ||
| 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" | ||
| "<available_skills>\n" | ||
| f"{skill_entries}\n" | ||
| "</available_skills>\n" | ||
| ) |
Contributor
There was a problem hiding this comment.
Escape metadata before embedding it in the XML-style prompt blocks.
Skill names, descriptions, and resource names come from SKILL.md and the filesystem, but they are interpolated verbatim into <available_skills>, <file>, and the name="..." attribute. A <, &, or quote in those values breaks the wrapper and can change the prompt structure. Please escape those fields, or validate them against a safe character set before formatting.
Also applies to: 202-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python-interpreter/packages/afm-core/src/afm/skills.py` around lines 176 -
199, The XML-style prompt blocks in build_skill_catalog are injecting unescaped
skill fields (SkillInfo.name, SkillInfo.description and any resource names)
directly into tags and attributes (<available_skills>, <skill>, <name>,
<description>, and the name="..." attribute used with activate_skill), so update
build_skill_catalog to escape or validate those values before formatting (use a
single util like xml_escape or html.escape to replace &, <, >, " and '). Also
apply the same escaping/validation to the other prompt-building code that
interpolates <file> elements and name="..." attributes so no user/FS-provided
string can break the wrapper or change prompt structure.
Comment on lines
+42
to
+80
| 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}" | ||
|
|
||
|
|
||
| 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}" |
Contributor
There was a problem hiding this comment.
Track activation state before allowing resource reads.
ActivateSkillTool never records which skills were activated, and ReadSkillResourceTool gets the full skills map up front, so any listed resource can be read directly without calling activate_skill first. That breaks the progressive-disclosure contract described in the tool docs/system prompt. Please share a per-runner activated_skills set between these tools and reject reads until the matching skill was successfully activated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py`
around lines 42 - 80, The tools currently allow reading any skill resource
because ActivateSkillTool does not record activations and ReadSkillResourceTool
uses the full skills map; add a shared per-runner set (e.g., activated_skills:
set[str]) accessible to both ActivateSkillTool and ReadSkillResourceTool, update
ActivateSkillTool._run to add the skill name into activated_skills after a
successful activate_skill(...) call, and modify ReadSkillResourceTool._run to
check the skill_name is present in activated_skills and raise/return an error if
not activated before calling read_skill_resource(...); reference the classes
ActivateSkillTool and ReadSkillResourceTool and functions activate_skill and
read_skill_resource when making the changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python-interpreter/packages/afm-core/src/afm/skills.py`:
- Around line 89-93: When detecting a single-skill file (the skill_md_path /
SKILL_FILE branch) wrap the parse_skill_md(skill_md_path, path) call in a
try/except ValueError block (mirroring the subdirectory handling at lines
107-111), log the parsing error with the same logger/message pattern used for
subdirectories, and on failure return an empty dict instead of letting the
ValueError propagate; this ensures parse_skill_md, SKILL_FILE, and skill_md_path
are handled consistently with the subdirectory parsing logic.
In `@python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py`:
- Around line 72-76: The _run method currently only catches ValueError but
read_skill_resource can raise UnicodeDecodeError when binary files are read;
update error handling either by adding an except UnicodeDecodeError as e branch
in _run (method _run in the class defined in skills.py) to return a clear error
string (e.g., "Error: {e}"), or modify read_skill_resource in afm/skills.py to
catch UnicodeDecodeError and re-raise it as a ValueError (raise ValueError(...)
from e) so the existing _run except ValueError path continues to work; choose
one of these fixes and apply consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17cff781-729b-4a2f-bf09-5fc1ab472c65
📒 Files selected for processing (4)
python-interpreter/packages/afm-core/src/afm/skills.pypython-interpreter/packages/afm-core/tests/test_skills.pypython-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.pypython-interpreter/packages/afm-langchain/tests/test_langchain_skills.py
✅ Files skipped from review due to trivial changes (2)
- python-interpreter/packages/afm-langchain/tests/test_langchain_skills.py
- python-interpreter/packages/afm-core/tests/test_skills.py
Comment on lines
+89
to
+93
| # 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} |
Contributor
There was a problem hiding this comment.
Inconsistent error handling for single-skill vs. subdirectory parsing.
When path/SKILL.md exists directly (single-skill case), parse_skill_md is called without a try-except, so a ValueError from malformed frontmatter will propagate up. In contrast, subdirectory skills (lines 107-111) catch ValueError and log an error, continuing gracefully. This inconsistency may cause unexpected crashes for invalid single-skill directories.
🛡️ Proposed fix to add consistent error handling
# 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}
+ try:
+ info = parse_skill_md(skill_md_path, path)
+ return {info.name: info}
+ except ValueError as e:
+ logger.error("Failed to parse skill at %s: %s", path, e)
+ return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python-interpreter/packages/afm-core/src/afm/skills.py` around lines 89 - 93,
When detecting a single-skill file (the skill_md_path / SKILL_FILE branch) wrap
the parse_skill_md(skill_md_path, path) call in a try/except ValueError block
(mirroring the subdirectory handling at lines 107-111), log the parsing error
with the same logger/message pattern used for subdirectories, and on failure
return an empty dict instead of letting the ValueError propagate; this ensures
parse_skill_md, SKILL_FILE, and skill_md_path are handled consistently with the
subdirectory parsing logic.
python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
python-interpreter/packages/afm-core/src/afm/skills.py (3)
162-172:⚠️ Potential issue | 🟠 MajorReject oversized resources before returning them to the model.
list_local_resources()advertises every file, andread_skill_resource()reads the entire file into memory and returns it verbatim. A large asset will go straight into the tool response, which can spike latency and context usage very quickly. Enforce a maximum size beforeread_text(), and ideally stop listing files above that threshold so the catalog matches what the tool can actually serve.Possible low-impact fix
logger = logging.getLogger(__name__) SKILL_FILE = "SKILL.md" REFERENCES_DIR = "references" ASSETS_DIR = "assets" +MAX_RESOURCE_BYTES = 64 * 1024 ... 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") + size = full_path.stat().st_size + if size > MAX_RESOURCE_BYTES: + raise ValueError( + f"Resource '{resource_path}' exceeds the {MAX_RESOURCE_BYTES}-byte limit" + ) + 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"Also applies to: 224-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/src/afm/skills.py` around lines 162 - 172, list_local_resources currently advertises every file even if too large for read_skill_resource; add a MAX_RESOURCE_SIZE_BYTES constant and use entry.stat().st_size to skip adding any file whose size exceeds that threshold so the catalog matches what can be served, and update read_skill_resource to check Path.stat().st_size before calling read_text() and raise/return a clear error (or None) for oversized files; reference the functions list_local_resources and read_skill_resource and perform the size check in both places using the same MAX_RESOURCE_SIZE_BYTES constant.
175-220:⚠️ Potential issue | 🟠 MajorEscape metadata before embedding it in the XML-style prompt blocks.
info.name,info.description, and resource names are interpolated raw into tags and attributes here. A<,&, or quote fromSKILL.mdor the filesystem can break the wrapper and alter the prompt structure.Possible low-impact fix
+from html import escape + ... skill_entries = "\n".join( f" <skill>\n" - f" <name>{info.name}</name>\n" - f" <description>{info.description}</description>\n" + f" <name>{escape(info.name)}</name>\n" + f" <description>{escape(info.description)}</description>\n" f" </skill>" for info in skills.values() ) ... if info.resources: - resource_entries = "\n".join(f"<file>{res}</file>" for res in info.resources) + resource_entries = "\n".join( + f"<file>{escape(res)}</file>" for res in info.resources + ) resources_section = ( f"\n<skill_resources>\n{resource_entries}\n</skill_resources>\n" "Use the read_skill_resource tool to read any of these files if needed.\n" ) return ( - f'<skill_content name="{info.name}">\n' + f'<skill_content name="{escape(info.name, quote=True)}">\n' f"{info.body}\n" f"{resources_section}" "</skill_content>\n" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/src/afm/skills.py` around lines 175 - 220, Escape XML/HTML-special characters before embedding skill metadata and resources into the XML-style blocks: update build_skill_catalog (the skill_entries construction that interpolates info.name and info.description) and activate_skill (the f-string that builds '<skill_content name="{info.name}">' and the resources_section/resource_entries that include resource names and info.body) to run values through an XML-escaping utility (e.g., html.escape or a small helper that replaces &, <, >, " and ') so names, descriptions, body text and filenames cannot break the wrapper or inject tags/attributes.
89-93:⚠️ Potential issue | 🟡 MinorDon't let one malformed direct skill source abort discovery.
Line 92 calls
parse_skill_md()outside theValueErrorguard used for subdirectory skills. If a configured source points directly at a skill folder, one badSKILL.mdwill abort discovery instead of being logged and skipped like the subdirectory case.Possible low-impact fix
skill_md_path = path / SKILL_FILE if skill_md_path.is_file(): - info = parse_skill_md(skill_md_path, path) - return {info.name: info} + try: + info = parse_skill_md(skill_md_path, path) + except ValueError as e: + logger.error("Failed to parse skill at %s: %s", path, e) + return {} + return {info.name: info}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/src/afm/skills.py` around lines 89 - 93, The direct-skill branch calls parse_skill_md(path / SKILL_FILE) without the ValueError guard used for subdirectory skills, so a malformed SKILL.md aborts discovery; wrap the call to parse_skill_md in a try/except ValueError (matching the handling used for subdirectory discovery), log the error with the same logger/format used elsewhere, and on exception skip the skill by returning an empty dict (or otherwise continuing) instead of letting the error propagate; target the code around skill_md_path, SKILL_FILE, parse_skill_md and mirror the existing subdirectory error handling.python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py (1)
40-76:⚠️ Potential issue | 🟠 MajorEnforce activation before serving skill resources.
ActivateSkillToolstill never records which skills were activated, andReadSkillResourceToolcan read any discovered resource as long as the caller knowsskill_nameandresource_path. Inpython-interpreter/packages/afm-langchain/src/afm_langchain/backend.pyLines 70-76, both tools are still constructed with onlyskills=skills, so the model can bypass the progressive-disclosure contract by callingread_skill_resourcefirst. Share a per-run activation state between these tools and reject reads until the matching skill has been activated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py` around lines 40 - 76, ActivateSkillTool currently doesn't record activations and ReadSkillResourceTool can be called without activation; add a shared per-run activation state (e.g., a set[str] named activated_skills) and pass it into both tool instances instead of only skills; modify ActivateSkillTool to add the activated skill to activated_skills after a successful call to activate_skill(name, self.skills) (use the ActivateSkillTool class and its _run method), and modify ReadSkillResourceTool._run to first check that skill_name is in activated_skills and raise/return an error if not before calling read_skill_resource(skill_name, resource_path, self.skills); update backend construction where the tools are instantiated so both get the same activated_skills object.
🧹 Nitpick comments (1)
ballerina-interpreter/parser.bal (1)
279-285: Improveskills.patherror precision with index-based keys.Current reporting always pushes
skills.path, which can be ambiguous when multiple skills are invalid.Proposed diff
- if skills is SkillSource[] { - foreach SkillSource skillSource in skills { - if containsHttpVariable(skillSource.path) { - erroredKeys.push("skills.path"); - } - } - } + if skills is SkillSource[] { + foreach int idx in 0 ..< skills.length() { + SkillSource skillSource = skills[idx]; + if containsHttpVariable(skillSource.path) { + erroredKeys.push(string `skills[${idx}].path`); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-interpreter/parser.bal` around lines 279 - 285, The current loop always pushes the generic key "skills.path", causing ambiguity when multiple SkillSource entries are invalid; change the iteration to capture the skill index (e.g., use an indexed for/foreach or iterate over 0..skills.length()-1) and when containsHttpVariable(skillSource.path) is true push an index-specific key such as "skills[<index>].path" (or `skills.<index>.path` if your project convention prefers) instead of the constant "skills.path" so each invalid skill is reported with its exact array index; update the block that uses skills, SkillSource, containsHttpVariable, and erroredKeys.push accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python-interpreter/packages/afm-core/src/afm/skills.py`:
- Around line 162-172: list_local_resources currently advertises every file even
if too large for read_skill_resource; add a MAX_RESOURCE_SIZE_BYTES constant and
use entry.stat().st_size to skip adding any file whose size exceeds that
threshold so the catalog matches what can be served, and update
read_skill_resource to check Path.stat().st_size before calling read_text() and
raise/return a clear error (or None) for oversized files; reference the
functions list_local_resources and read_skill_resource and perform the size
check in both places using the same MAX_RESOURCE_SIZE_BYTES constant.
- Around line 175-220: Escape XML/HTML-special characters before embedding skill
metadata and resources into the XML-style blocks: update build_skill_catalog
(the skill_entries construction that interpolates info.name and
info.description) and activate_skill (the f-string that builds '<skill_content
name="{info.name}">' and the resources_section/resource_entries that include
resource names and info.body) to run values through an XML-escaping utility
(e.g., html.escape or a small helper that replaces &, <, >, " and ') so names,
descriptions, body text and filenames cannot break the wrapper or inject
tags/attributes.
- Around line 89-93: The direct-skill branch calls parse_skill_md(path /
SKILL_FILE) without the ValueError guard used for subdirectory skills, so a
malformed SKILL.md aborts discovery; wrap the call to parse_skill_md in a
try/except ValueError (matching the handling used for subdirectory discovery),
log the error with the same logger/format used elsewhere, and on exception skip
the skill by returning an empty dict (or otherwise continuing) instead of
letting the error propagate; target the code around skill_md_path, SKILL_FILE,
parse_skill_md and mirror the existing subdirectory error handling.
In `@python-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py`:
- Around line 40-76: ActivateSkillTool currently doesn't record activations and
ReadSkillResourceTool can be called without activation; add a shared per-run
activation state (e.g., a set[str] named activated_skills) and pass it into both
tool instances instead of only skills; modify ActivateSkillTool to add the
activated skill to activated_skills after a successful call to
activate_skill(name, self.skills) (use the ActivateSkillTool class and its _run
method), and modify ReadSkillResourceTool._run to first check that skill_name is
in activated_skills and raise/return an error if not before calling
read_skill_resource(skill_name, resource_path, self.skills); update backend
construction where the tools are instantiated so both get the same
activated_skills object.
---
Nitpick comments:
In `@ballerina-interpreter/parser.bal`:
- Around line 279-285: The current loop always pushes the generic key
"skills.path", causing ambiguity when multiple SkillSource entries are invalid;
change the iteration to capture the skill index (e.g., use an indexed
for/foreach or iterate over 0..skills.length()-1) and when
containsHttpVariable(skillSource.path) is true push an index-specific key such
as "skills[<index>].path" (or `skills.<index>.path` if your project convention
prefers) instead of the constant "skills.path" so each invalid skill is reported
with its exact array index; update the block that uses skills, SkillSource,
containsHttpVariable, and erroredKeys.push accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58fe104d-a774-48b3-b534-086129599898
📒 Files selected for processing (5)
ballerina-interpreter/parser.balballerina-interpreter/skills.balpython-interpreter/packages/afm-core/src/afm/models.pypython-interpreter/packages/afm-core/src/afm/skills.pypython-interpreter/packages/afm-langchain/src/afm_langchain/tools/skills.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ballerina-interpreter/skills.bal
RadCod3
approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
$title.
PR from feature branch with #13 and #15.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores