diff --git a/helpers/skills.py b/helpers/skills.py index 035fe089f..74488da9d 100644 --- a/helpers/skills.py +++ b/helpers/skills.py @@ -25,7 +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() +_WARNED_SKILL_PARSE_PATHS: set[Path] = set() class ActiveSkillEntry(TypedDict, total=False): @@ -264,9 +264,7 @@ def _emit_skill_scan_warning(message: str) -> None: print(f"Warning: {message}") -def _frontmatter_error_line(markdown: str, error: str) -> int: - text = markdown or "" - lines = text.splitlines() +def _frontmatter_error_line(lines: List[str], error: str) -> int | None: if not lines: return 1 @@ -279,30 +277,22 @@ def _frontmatter_error_line(markdown: str, error: str) -> int: 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 + return None 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: + if skill_md_path in _WARNED_SKILL_PARSE_PATHS: return - _SKILL_PARSE_WARNINGS.add(key) + _WARNED_SKILL_PARSE_PATHS.add(skill_md_path) + + error = str(errors[0] or "invalid frontmatter").strip() + line = _frontmatter_error_line((markdown or "").splitlines(), error) skill_label = skill_md_path.parent.name or str(skill_md_path) + location = f" at line {line}" if line is not None else "" _emit_skill_scan_warning( - f"skill {skill_label} skipped: invalid frontmatter at line {line}: {error}" + f"skill {skill_label} skipped: invalid frontmatter{location}: {error}" ) diff --git a/helpers/skills.py.dox.md b/helpers/skills.py.dox.md index 6cc9090fa..d269bb205 100644 --- a/helpers/skills.py.dox.md +++ b/helpers/skills.py.dox.md @@ -56,7 +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. +- Invalid `SKILL.md` frontmatter emits a once-per-path scan warning with the skipped skill path/name and a line number when the parser can identify one directly. - 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 f4d35684b..aa988ba2d 100644 --- a/tests/test_skills_runtime.py +++ b/tests/test_skills_runtime.py @@ -341,14 +341,15 @@ def test_invalid_skill_frontmatter_warns_when_skill_is_skipped(monkeypatch, tmp_ ) warnings: list[str] = [] - runtime._SKILL_PARSE_WARNINGS.clear() + runtime._WARNED_SKILL_PARSE_PATHS.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 warnings == [ + "skill broken-skill skipped: invalid frontmatter at line 4: " + "Unterminated YAML frontmatter" + ] assert runtime.list_skills() == [] assert len(warnings) == 1