-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Chatagent skills #3732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Chatagent skills #3732
Conversation
…nto chatagent-skill
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
Why don’t we implement skills as tools like I mentioned? |
I've surveyed several repos; some use tools, some don't. I think each approach likely has its advantages, and perhaps we can discuss it and decide which method to adopt. |
waleedalzarooni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just one quick comment
camel/toolkits/skill_toolkit.py
Outdated
| ) | ||
|
|
||
| def _get_skills(self) -> Dict[str, Dict[str, str]]: | ||
| self._skills_cache = self._scan_skills() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no check whether self._skills_cache is not none, in cases where there are repeated skill related tool calls this could result in unnecessary scans within a given session. Consider adding if self._skills_cache is None: to prevent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now using cache will prevent the case for example agent create new skill on the fly the reuse the skill right? Maybe we should remove it for now. WDYT?
zechengz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add some unit tests
camel/toolkits/skill_toolkit.py
Outdated
| f"({skill['path']})\n" | ||
| ) | ||
| lines.append("</available_skills>") | ||
| if not skills: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this below the skills = self._get_skills()
| skills = self._get_skills() | ||
| skill = skills.get(name) | ||
| if not skill: | ||
| available = ", ".join(sorted(skills.keys())) or "none" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also includes the description of path to each skills?
camel/toolkits/skill_toolkit.py
Outdated
| roots.append(("system", codex_home_path / "skills" / ".system")) | ||
| return roots | ||
|
|
||
| def _find_repo_root(self) -> Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the assumption of .git is incorrect here
either we just use working directory or let user specify another directory for the repo / workspace scope skill
|
|
||
| try: | ||
| contents = path.read_text(encoding="utf-8") | ||
| except OSError as exc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can do this
define a general skill error class
class SkillError(Exception)and we can have
SkillFrontmatterMissingetc inherit SkillError
then at the place we call _parse_skill (just returns dict[str, str])
we have something like
try:
skill = _parse_skill(path)
except SkillError as e:
logger.warning("Skip %s: %s", path, e)
continueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be over-engineering for this use case as _parse_skill is an internal method only called by _scan_skills, so detailed exception types aren't exposed to external callers
camel/toolkits/skill_toolkit.py
Outdated
| info = self._parse_skill(path) | ||
| if not info: | ||
| continue | ||
| name = info["name"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the scope related overwriting more explicit here? and add some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can have a folder in examples toolkits called skill_toolkit with a demo skill and a python script
camel/toolkits/skill_toolkit.py
Outdated
|
|
||
| import os | ||
| from pathlib import Path | ||
| from typing import Dict, List, Optional, Tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use dict, list, tuple and
dict[str, str] | None
instead of import from typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's good for now, as camel repo uses typing a lot, let's unify the code style with typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1
Wendong-Fan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice implementation! Found a couple small issues noted inline. Also +1 to adding unit tests as @zechengz mentioned.
camel/toolkits/skill_toolkit.py
Outdated
| ) | ||
|
|
||
| def _get_skills(self) -> Dict[str, Dict[str, str]]: | ||
| self._skills_cache = self._scan_skills() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overwrites the cache on every call, so it never actually uses the cached data. Should check if cache exists first:
if self._skills_cache is None:
self._skills_cache = self._scan_skills()
return self._skills_cache
camel/toolkits/skill_toolkit.py
Outdated
| "- Safety and fallback: If a skill can't be applied cleanly " | ||
| "(missing files, unclear instructions), state the issue, pick the " | ||
| "next-best approach, and continue.\n" | ||
| "\n" + " ".join(lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using space join here makes the output hard to read since all the XML content ends up on one line. Consider using "\n".join() instead for better readability.
Wendong-Fan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Describe your changes in detail (optional if the linked issue already contains a detailed description of the changes).
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!