Add plugin-bundled skills discovery and install CLI#7289
Add plugin-bundled skills discovery and install CLI#7289AdonaiVera wants to merge 4 commits intodevelopfrom
Conversation
WalkthroughAdds a new "skills" subsystem for plugins: a SkillDefinition model and a public list_skills(enabled=True, plugin=None, category=None) function that discovers SKILL.md files declared in plugin metadata, parses optional YAML frontmatter for name/category/description, and yields SkillDefinition instances. Exposes list_skills from fiftyone.plugins and adds a Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "fiftyone plugins skills list"
participant ListSkills as list_skills()
participant Plugins as Plugin Registry
participant FS as File System
User->>CLI: run list (--plugin/--category/--names-only)
CLI->>ListSkills: list_skills(plugin, category)
ListSkills->>Plugins: list_plugins(enabled=True, builtin="all")
Plugins-->>ListSkills: [PluginDefinition...]
loop per plugin
ListSkills->>FS: resolve declared SKILL.md paths
FS-->>ListSkills: file content or missing
ListSkills->>ListSkills: parse YAML frontmatter -> metadata
ListSkills->>ListSkills: construct SkillDefinition, apply filters
end
ListSkills-->>CLI: [SkillDefinition...]
CLI-->>User: print names or table
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiftyone/core/cli.py`:
- Around line 4595-4600: The CLI argument definition for "--plugin-names" (in
fiftyone/core/cli.py) uses nargs="*" which allows an empty list (e.g.,
`--plugin-names` -> []), causing silent no-op installs; change to nargs="+" to
require at least one value, or detect an empty list after parsing and normalize
it to None before downstream filtering (update the parser entry for
"--plugin-names" or the code that reads plugin_names to treat [] as None).
In `@fiftyone/plugins/skills.py`:
- Around line 51-56: The name() method currently falls back to the file basename
(which yields "SKILL" for skills/<slug>/SKILL.md) causing ambiguous names;
update the fallback in name(self) to use the parent directory name instead
(e.g., os.path.basename(os.path.dirname(self._path))) so unnamed skills use
their skill directory slug; modify the return in name() to first try
self._metadata["name"] and otherwise return the parent directory basename.
- Around line 207-215: The code currently trusts plugin_definition.skills
entries verbatim, allowing paths outside the plugin directory; update
_iter_plugin_skills to canonicalize and validate each skill path before reading:
compute skill_path = os.path.abspath(os.path.join(plugin_definition.directory,
rel_path)) and ensure it lies inside the plugin directory (e.g., compare
os.path.commonpath([plugin_definition.directory, skill_path]) ==
os.path.abspath(plugin_definition.directory)); if the check fails or the path is
absolute/attempts traversal, log/debug a rejection and continue without calling
_parse_skill_frontmatter or yielding a SkillDefinition to prevent reading
arbitrary local files.
- Around line 178-199: skill.name is used directly to build paths
(skill_central, link_path) allowing absolute names or '..' segments to escape
managed directories; validate and sanitize it before joining: ensure skill.name
is not absolute (use os.path.isabs), does not contain path separators or parent
segments (no '..'), and is equal to a safe basename (e.g., compare to
os.path.basename(skill.name)); if validation fails, skip or raise a clear error
and log it. Apply this check before computing skill_central, dest, and link_path
in the loop that references skill.name so only safe directory names are
created/copied and symlinked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97f9143b-5cff-4b4c-9410-5f2fe8a9db69
📒 Files selected for processing (5)
fiftyone/core/cli.pyfiftyone/plugins/__init__.pyfiftyone/plugins/definitions.pyfiftyone/plugins/skills.pytests/unittests/plugins/skills_tests.py
kaixi-wang
left a comment
kaixi-wang
left a comment
There was a problem hiding this comment.
Not sure if this intended for oss local only, but this may not work in teams as is due to the writing of files and reliance on local files
|
Hi @kaixi-wang Just to clarify, the skill installation part is CLI-only for open source. In enterprise, we’re not installing skills directly to a specific agent since we already have our own infrastructure handling that. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiftyone/plugins/skills.py`:
- Around line 186-191: The current copy/symlink logic silently overwrites
existing skills when two plugins share the same skill.name; change it to detect
conflicts and avoid overwriting: before os.makedirs/ shutil.copy2 (references:
variables skill_central, dest, and skill.path) check whether skill_central or
dest already exists and if the existing content comes from a different plugin
(use the plugin identifier/source available on the skill object), then do not
overwrite—log an error/warning including both plugin identities and skip or
create a namespaced directory instead (e.g., append plugin id or timestamp) so
installations do not clobber each other; apply the same conflict check and
behavior to the symlink creation code that uses skill.name as the key (the block
around the symlink handling referenced at Lines ~199-209).
- Around line 200-204: The cleanup before creating symlinks in the function
handling link_path (in fiftyone/plugins/skills.py) only removes symlinks and
directories, causing os.symlink() to raise FileExistsError when a regular file
exists; update that block to also detect and remove regular files (use
os.path.isfile or os.path.exists with not islink/isdir) and call
os.remove(link_path) before creating the symlink so all pre-existing file types
are handled safely.
- Around line 131-133: Replace the bare except with "except Exception:" and log
the full traceback using logger.exception so failures are diagnosable;
specifically, in the block that currently reads "except:" referencing the plugin
descriptor variable pd (pd.name), change it to "except Exception:" and call
logger.exception with a message that includes pd.name so the stack trace is
recorded while the function can continue skipping that plugin.
- Around line 250-252: The YAML frontmatter parser _parse_skill_frontmatter
should guard against non-dict results from yaml.safe_load to avoid runtime
crashes when SkillDefinition later calls .get(); change the logic in
_parse_skill_frontmatter to assign the result of yaml.safe_load(parts[1]) to a
variable, check isinstance(result, dict) and return result if true, otherwise
return an empty dict (still catching yaml.YAMLError as before), so malformed
frontmatter like a number or list won't propagate a non-dict into
SkillDefinition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75f48638-3f69-473d-93ff-ef4d616e5c1f
📒 Files selected for processing (2)
fiftyone/core/cli.pyfiftyone/plugins/skills.py
@ritch I'm not moving the skills. It is just copying the skill to a specific folder so agents like Claude or Cursor can use it. The skill still stays defined in the fiftyone.yml file. Then, when the user runs it locally with DevAgent, it gets added to that user’s environment and becomes available there. |
Burhan-Q
left a comment
Burhan-Q
left a comment
There was a problem hiding this comment.
I stopped making pathlib.Path suggestions after checking that the ~ handling works on Windows. Still, I would love to see its adoption, but not at all blocking. Couple other minor nits, but overall looks like a solid feature add
| for s in skills: | ||
| print(s.name) | ||
|
|
||
| return |
There was a problem hiding this comment.
What's the Voxel51 consensus on "naked" return statements? Personally, I'm not a fan, but I'm not saying this has to change, just looking to ensure that I understand the established convention for the repo.
There was a problem hiding this comment.
Just following the FO style, not my preference.
|
|
||
| return | ||
|
|
||
| headers = ["name", "category", "plugin", "path"] |
There was a problem hiding this comment.
Wonder if a "last updated" or "last downloaded" column might make sense? Not clear how feasible that would be, but maybe something to consider
There was a problem hiding this comment.
Oh I think this could be super cool, but it should be part of the plugin framework itself. It’s out of scope for this PR.
| """The name of the skill.""" | ||
| return self._metadata.get( | ||
| "name", | ||
| os.path.basename(os.path.dirname(self._path)), |
There was a problem hiding this comment.
| os.path.basename(os.path.dirname(self._path)), | |
| self._path.parent.name, |
There was a problem hiding this comment.
If you opt to take my suggestion for using pathlib.Path
There was a problem hiding this comment.
Thanks for the suggestion! I kept SkillDefinition consistent with PluginDefinition, which uses os.path strings. Using pathlib only here would make them inconsistent.
| """ | ||
|
|
||
| def __init__(self, path, metadata, plugin_name): | ||
| self._path = path |
There was a problem hiding this comment.
| self._path = path | |
| self._path = Path(path).absolute() |
|
I removed the CLI agent installation part. For reference that was just a helper to copy our skills from the plugin to agents like Claude, Codex, Cursor, etc. I moved that out to keep this PR clean and focused only on the skill listing functionality @ritch |
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 `@fiftyone/core/cli.py`:
- Around line 4491-4508: The CLI currently only accepts single values for
--plugin and --category; update the parser.add_argument calls for the
"-p/--plugin" and "-c/--category" options to accept multiple values (e.g., add
nargs="+" or action="append") so they produce lists, and make sure the code that
calls list_skills(plugin=..., category=...) passes those lists (if using
action="append" flatten the resulting list) so list_skills() receives the
expected list types while leaving the "-n/--names-only" flag unchanged.
In `@fiftyone/plugins/skills.py`:
- Around line 63-64: The file reads use the system default encoding; update the
file-open calls to specify encoding='utf-8' to ensure cross-platform
consistency: change the open in the method that reads the skill markdown (the
with open(self._path, "r") as f block) to include encoding='utf-8', and make the
same change in the _parse_skill_frontmatter function where it opens/reads the
skill file so both places explicitly use UTF-8.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba311043-0113-4653-88a9-dd2daab7e879
📒 Files selected for processing (4)
fiftyone/core/cli.pyfiftyone/plugins/__init__.pyfiftyone/plugins/skills.pytests/unittests/plugins/skills_tests.py
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 `@fiftyone/plugins/skills.py`:
- Around line 148-157: The current frontmatter detection uses
content.startswith("---") and content.split("---", 2), which can match `---`
inside YAML values; update parsing to only treat `---` as delimiters when they
appear on their own line. Modify the logic around the variable content (where
parts = content.split...) to locate the first and second delimiter by searching
for lines that match exactly `---` (e.g., using a line-scan or a multiline regex
like `(?m)^---\s*$`), extract the text between those two delimiter lines, then
call yaml.safe_load on that extracted block and return it if it is a dict.
Ensure yaml.safe_load(parts[1]) is replaced with loading the extracted
frontmatter block and retain the existing fallback to return {} for malformed or
missing frontmatter.
- Around line 107-115: The plugin-skill discovery currently catches exceptions
around the whole per-plugin loop, so a single bad skill aborts loading all
remaining skills from that plugin; change the error handling to catch exceptions
per-skill inside _iter_plugin_skills (or inside the inner loop over
plugin_definition.skills) so that on exception you log which rel_path and
plugin_definition.name failed and continue to the next skill (i.e., move the
try/except into the inner loop and keep yielding valid SkillDefinition instances
from _iter_plugin_skills).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52c9a1df-e84d-44eb-a97a-5050c3d5b862
📒 Files selected for processing (2)
fiftyone/core/cli.pyfiftyone/plugins/skills.py
| for pd in plugin_defs: | ||
| try: | ||
| for skill in _iter_plugin_skills(pd): | ||
| if category is not None and skill.category not in category: | ||
| continue | ||
| skills.append(skill) | ||
| except: | ||
| logger.info(f"Failed to load skills from plugin '{pd.name}'") | ||
|
|
There was a problem hiding this comment.
Handle failures per skill, not per plugin.
The try here aborts discovery of every remaining skill in a plugin after the first bad entry. A single unreadable file or malformed rel_path should be skipped without hiding valid sibling skills.
Suggested fix
- for pd in plugin_defs:
- try:
- for skill in _iter_plugin_skills(pd):
- if category is not None and skill.category not in category:
- continue
- skills.append(skill)
- except:
- logger.info(f"Failed to load skills from plugin '{pd.name}'")
+ for pd in plugin_defs:
+ for skill in _iter_plugin_skills(pd):
+ if category is not None and skill.category not in category:
+ continue
+ skills.append(skill)def _iter_plugin_skills(plugin_definition):
plugin_dir = os.path.realpath(plugin_definition.directory)
for rel_path in plugin_definition.skills:
try:
skill_path = os.path.realpath(os.path.join(plugin_dir, rel_path))
# existing validation...
metadata = _parse_skill_frontmatter(skill_path)
yield SkillDefinition(skill_path, metadata, plugin_definition.name)
except:
logger.info(
f"Failed to load skill '{rel_path}' from plugin "
f"'{plugin_definition.name}'"
)
continue🧰 Tools
🪛 Ruff (0.15.7)
[error] 113-113: Do not use bare except
(E722)
[warning] 114-114: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fiftyone/plugins/skills.py` around lines 107 - 115, The plugin-skill
discovery currently catches exceptions around the whole per-plugin loop, so a
single bad skill aborts loading all remaining skills from that plugin; change
the error handling to catch exceptions per-skill inside _iter_plugin_skills (or
inside the inner loop over plugin_definition.skills) so that on exception you
log which rel_path and plugin_definition.name failed and continue to the next
skill (i.e., move the try/except into the inner loop and keep yielding valid
SkillDefinition instances from _iter_plugin_skills).
| if not content.startswith("---"): | ||
| return {} | ||
|
|
||
| parts = content.split("---", 2) | ||
| if len(parts) < 3: | ||
| return {} | ||
|
|
||
| try: | ||
| result = yaml.safe_load(parts[1]) | ||
| return result if isinstance(result, dict) else {} |
There was a problem hiding this comment.
Parse frontmatter delimiters as standalone lines.
startswith("---") plus split("---", 2) stops at the next --- anywhere in the file. Valid YAML like description: "foo --- bar" or block scalars containing --- will be truncated, which silently drops metadata and can break --category filtering.
Suggested fix
- if not content.startswith("---"):
+ lines = content.splitlines(keepends=True)
+ if not lines or lines[0].strip() != "---":
return {}
- parts = content.split("---", 2)
- if len(parts) < 3:
+ frontmatter = None
+ for idx, line in enumerate(lines[1:], 1):
+ if line.strip() == "---":
+ frontmatter = "".join(lines[1:idx])
+ break
+
+ if frontmatter is None:
return {}
try:
- result = yaml.safe_load(parts[1])
+ result = yaml.safe_load(frontmatter)
return result if isinstance(result, dict) else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fiftyone/plugins/skills.py` around lines 148 - 157, The current frontmatter
detection uses content.startswith("---") and content.split("---", 2), which can
match `---` inside YAML values; update parsing to only treat `---` as delimiters
when they appear on their own line. Modify the logic around the variable content
(where parts = content.split...) to locate the first and second delimiter by
searching for lines that match exactly `---` (e.g., using a line-scan or a
multiline regex like `(?m)^---\s*$`), extract the text between those two
delimiter lines, then call yaml.safe_load on that extracted block and return it
if it is a dict. Ensure yaml.safe_load(parts[1]) is replaced with loading the
extracted frontmatter block and retain the existing fallback to return {} for
malformed or missing frontmatter.
4 participants
📋 What changes are proposed in this pull request?
This PR adds support for skills inside plugins in FiftyOne, aligned with the data agent work.
The idea is pretty simple. Skills are just Markdown files for AI agents, and now you can define them directly in
fiftyone.yml, the same way we already do with operators and panels. Sofiftyone.ymlbecomes the source of truth, and you just list your skills underskills:using relative paths. Plugins can even be just skills, no operators needed.🧪 How is this patch tested? If it is not, please explain why.
pre-commit run --filespassespytest tests/unittests/plugins/fiftyone plugins skills listshows skillsSome example for testing
📝 Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Adds support for plugin-based AI agent skills in FiftyOne, including CLI commands to list skills.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Tests