diff --git a/helpers/skills.py b/helpers/skills.py index 09d178817..035fe089f 100644 --- a/helpers/skills.py +++ b/helpers/skills.py @@ -25,6 +25,7 @@ CONTEXT_DATA_NAME_LOADED_SKILLS = AGENT_DATA_NAME_LOADED_SKILLS CONTEXT_DATA_NAME_CHAT_ACTIVE_SKILLS = "skills_chat_active" CONTEXT_DATA_NAME_CHAT_DISABLED_SKILLS = "skills_chat_disabled" CONTEXT_DATA_NAME_CHAT_VISIBLE_SKILLS = "skills_chat_visible" +_SKILL_PARSE_WARNINGS: set[str] = set() class ActiveSkillEntry(TypedDict, total=False): @@ -254,6 +255,57 @@ def parse_frontmatter(frontmatter_text: str) -> Tuple[Dict[str, Any], List[str]] return parsed, errors +def _emit_skill_scan_warning(message: str) -> None: + try: + from helpers.print_style import PrintStyle + + PrintStyle.warning(message) + except Exception: + print(f"Warning: {message}") + + +def _frontmatter_error_line(markdown: str, error: str) -> int: + text = markdown or "" + lines = text.splitlines() + if not lines: + return 1 + + if error.startswith("Frontmatter must start"): + for index, line in enumerate(lines, start=1): + if line.strip(): + return index + return 1 + if error.startswith("Missing YAML frontmatter"): + return 1 + if error.startswith("Unterminated YAML frontmatter"): + return max(len(lines), 1) + + match = re.search(r"line\s+(\d+)", error, flags=re.IGNORECASE) + if match: + start_idx = 0 + for index, line in enumerate(lines): + if line.strip() == "---": + start_idx = index + break + return start_idx + int(match.group(1)) + 1 + return 1 + + +def _warn_skill_skipped(skill_md_path: Path, markdown: str, errors: List[str]) -> None: + if not errors: + return + error = str(errors[0] or "invalid frontmatter").strip() + line = _frontmatter_error_line(markdown, error) + key = f"{skill_md_path}:{line}:{error}" + if key in _SKILL_PARSE_WARNINGS: + return + _SKILL_PARSE_WARNINGS.add(key) + skill_label = skill_md_path.parent.name or str(skill_md_path) + _emit_skill_scan_warning( + f"skill {skill_label} skipped: invalid frontmatter at line {line}: {error}" + ) + + def skill_from_markdown( skill_md_path: Path, *, @@ -267,6 +319,7 @@ def skill_from_markdown( fm, body, fm_errors = split_frontmatter(text) if fm_errors: + _warn_skill_skipped(skill_md_path, text, fm_errors) return None skill_dir = Path(files.normalize_a0_path(str(skill_md_path.parent))) diff --git a/helpers/skills.py.dox.md b/helpers/skills.py.dox.md index 19e337c0b..6cc9090fa 100644 --- a/helpers/skills.py.dox.md +++ b/helpers/skills.py.dox.md @@ -56,6 +56,7 @@ - Helper modules own reusable framework APIs and must preserve public callers unless all callers, tests, and docs are updated together. - Update this file whenever public functions, classes, persistence behavior, path/security assumptions, side effects, or cross-module contracts change. - Loaded skill names are chat-wide context data under `CONTEXT_DATA_NAME_LOADED_SKILLS`; legacy agent-local `loaded_skills` lists are migrated into context data and cleared when read. +- Invalid `SKILL.md` frontmatter emits a deduplicated scan warning with the skipped skill path/name and best-effort line number instead of disappearing silently from lists, search, catalog, and load. - Observed side-effect areas: filesystem reads, filesystem deletion, plugin state, settings/state persistence, context data, secret handling. - Imported dependency areas include: `__future__`, `dataclasses`, `helpers`, `os`, `pathlib`, `re`, `typing`. diff --git a/tests/test_skills_runtime.py b/tests/test_skills_runtime.py index 068a93c5b..f4d35684b 100644 --- a/tests/test_skills_runtime.py +++ b/tests/test_skills_runtime.py @@ -331,6 +331,29 @@ def test_invalid_skill_frontmatter_reports_yaml_errors(): assert errors[0].startswith("Invalid YAML frontmatter") +def test_invalid_skill_frontmatter_warns_when_skill_is_skipped(monkeypatch, tmp_path: Path): + skills_root = tmp_path / "skills" + broken = skills_root / "broken-skill" + broken.mkdir(parents=True) + (broken / "SKILL.md").write_text( + "---\nname: broken-skill\ndescription: missing closing fence\nBody\n", + encoding="utf-8", + ) + + warnings: list[str] = [] + runtime._SKILL_PARSE_WARNINGS.clear() + monkeypatch.setattr(runtime, "get_skill_roots", lambda agent=None: [str(skills_root)]) + monkeypatch.setattr(runtime, "_emit_skill_scan_warning", warnings.append) + + assert runtime.list_skills() == [] + assert len(warnings) == 1 + assert "skill broken-skill skipped: invalid frontmatter at line 4" in warnings[0] + assert "Unterminated YAML frontmatter" in warnings[0] + + assert runtime.list_skills() == [] + assert len(warnings) == 1 + + def test_a0_manage_plugin_skill_frontmatter_is_valid_yaml(): text = (PROJECT_ROOT / "skills" / "a0-manage-plugin" / "SKILL.md").read_text( encoding="utf-8" diff --git a/tests/test_tool_action_contracts.py b/tests/test_tool_action_contracts.py index a38d7079e..4e964130f 100644 --- a/tests/test_tool_action_contracts.py +++ b/tests/test_tool_action_contracts.py @@ -273,6 +273,42 @@ def test_skills_tool_accepts_action_alias_for_search(monkeypatch, tmp_path: Path assert "browser-form-workflows" in response.message +def test_skills_tool_accepts_method_as_deprecated_action_alias( + monkeypatch, tmp_path: Path +): + module = _load_skills_tool(monkeypatch, tmp_path) + tool = module.SkillsTool( + _FakeAgent(), + "skills_tool", + None, + {"method": "search", "query": "browser forms"}, + "", + None, + ) + + response = asyncio.run(tool.execute(**tool.args)) + + assert "browser-form-workflows" in response.message + assert tool.args["action"] == "search" + + +def test_skills_tool_defaults_missing_action_to_list(monkeypatch, tmp_path: Path): + module = _load_skills_tool(monkeypatch, tmp_path) + tool = module.SkillsTool( + _FakeAgent(), + "skills_tool", + None, + {}, + "", + None, + ) + + response = asyncio.run(tool.execute()) + + assert "Available skills" in response.message + assert "browser-form-workflows" in response.message + + def test_skills_tool_load_appends_skill_instructions_as_tool_result( monkeypatch, tmp_path: Path ): diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 131d416d7..99628b7a0 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -24,17 +24,26 @@ class SkillsTool(Tool): Script execution is handled by code_execution_tool directly. """ - def _current_action(self) -> str: + @staticmethod + def _normalize_action(action: object) -> str: return ( str( - self.args.get("action") - or "" + action + or "list" ) .strip() .lower() .replace("-", "_") ) + def _current_action(self, **kwargs) -> str: + return self._normalize_action( + kwargs.get("action") + or self.args.get("action") + or kwargs.get("method") + or self.args.get("method") + ) + @staticmethod def _normalize_skill_name(skill_name: str) -> str: skill_name = skill_name.strip() @@ -65,14 +74,14 @@ class SkillsTool(Tool): return super().get_log_object() async def before_execution(self, **kwargs): - if self._current_action() != "load": + if self._current_action(**kwargs) != "load": await super().before_execution(**kwargs) return skill_name = self._normalize_skill_name( str(kwargs.get("skill_name") or self.args.get("skill_name") or "") ) - label = f"{self.name} action {self._current_action()}" + label = f"{self.name} action {self._current_action(**kwargs)}" if skill_name: PrintStyle( font_color="#1B4F72", @@ -90,35 +99,29 @@ class SkillsTool(Tool): self.log = self.get_log_object() async def execute(self, **kwargs) -> Response: - action = ( - str( - kwargs.get("action") - or self.args.get("action") - or "" - ) - .strip() - .lower() - .replace("-", "_") + action = self._current_action(**kwargs) + + query = str(kwargs.get("query") or self.args.get("query") or "").strip() + skill_name = self._normalize_skill_name( + str(kwargs.get("skill_name") or self.args.get("skill_name") or "") ) + file_path = str( + kwargs.get("file_path") or self.args.get("file_path") or "" + ).strip() + + if "action" not in kwargs and "action" not in self.args and "method" in kwargs: + kwargs["action"] = action + if "action" not in self.args and "method" in self.args: + self.args["action"] = action try: if action == "list": return Response(message=self._list(), break_loop=False) if action == "search": - query = str(kwargs.get("query") or self.args.get("query") or "").strip() return Response(message=self._search(query), break_loop=False) if action == "load": - skill_name = self._normalize_skill_name( - str(kwargs.get("skill_name") or self.args.get("skill_name") or "") - ) return self._load(skill_name) if action == "read_file": - skill_name = self._normalize_skill_name( - str(kwargs.get("skill_name") or self.args.get("skill_name") or "") - ) - file_path = str( - kwargs.get("file_path") or self.args.get("file_path") or "" - ).strip() return Response( message=self._read_file(skill_name, file_path), break_loop=False, diff --git a/tools/skills_tool.py.dox.md b/tools/skills_tool.py.dox.md index fcc023911..1c3e70e07 100644 --- a/tools/skills_tool.py.dox.md +++ b/tools/skills_tool.py.dox.md @@ -29,6 +29,7 @@ - Loading a skill appends the full skill body as a normal tool-result history message with `skill_instructions` metadata containing name, path, source, and content visibility. - Loaded skill IDs are stored in chat-wide context data. - Duplicate loads omit the full body when the same skill name remains visible in model history. +- Missing or empty `action` defaults to `list`, and legacy `method` is accepted as a deprecated alias when `action` is absent. - Observed side-effect areas: filesystem reads, filesystem deletion, settings/state persistence, chat history persistence. - Imported dependency areas include: `__future__`, `helpers`, `helpers.print_style`, `helpers.tool`, `pathlib`, `typing`.