-
Notifications
You must be signed in to change notification settings - Fork 2
#940 Added shared validation for packaged skills #954
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
Changes from all commits
8f8a001
66b9494
70b89ce
ac8a485
f411302
5932fee
4f4697c
70d1ff7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,7 @@ | ||
| # Unreleased | ||
|
|
||
| ## Features | ||
|
|
||
| - #940: Added shared validation for packaged agent skills and the `skills:check` Nox session. | ||
|
|
||
| ## Summary |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| .. _agent_skills: | ||
|
|
||
| Agent Skills | ||
| ============ | ||
|
|
||
| The PTB can package agent skills for use by projects and provides shared | ||
| validation for their common structure and content rules. | ||
|
|
||
| Run the validation with: | ||
|
|
||
| .. code-block:: shell | ||
|
|
||
| poetry run -- nox -s skills:check | ||
|
|
||
| The session validates every skill packaged in ``exasol.toolbox.skills``. It | ||
| checks that each skill has ``SKILL.md`` with complete frontmatter, contains no | ||
| unfinished TODO markers or forbidden repository-specific metadata, and has no | ||
| duplicated Markdown lines. Nox command examples are kept in the skill's | ||
| ``references/nox-sessions.md`` file. | ||
|
|
||
| These shared checks are intentionally separate from skill-specific tests. When | ||
| adding a skill, add its expected files and behavior assertions to that skill's | ||
| own test module, while ``skills:check`` covers the rules common to all skills. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| """Nox sessions for validating packaged agent skills.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import nox | ||
| from nox import Session | ||
|
|
||
| from exasol.toolbox.util.skills import ( | ||
| get_packaged_skill_names, | ||
| validate_skill, | ||
| ) | ||
|
|
||
|
|
||
| def _format_skill_errors(skill_name: str, errors: tuple[str, ...]) -> str: | ||
| """Format validation errors for one skill.""" | ||
| error_list = "\n".join(f" - {error}" for error in errors) | ||
| return f"{skill_name}:\n{error_list}" | ||
|
|
||
|
|
||
| @nox.session(name="skills:check", python=False) | ||
| def check_skills(session: Session) -> None: | ||
| """Validate the common structure and content rules for packaged skills.""" | ||
| failures = { | ||
| skill_name: validate_skill(skill_name) | ||
| for skill_name in get_packaged_skill_names() | ||
| } | ||
| failures = {skill_name: errors for skill_name, errors in failures.items() if errors} | ||
| if failures: | ||
| details = "\n".join( | ||
| _format_skill_errors(skill_name, errors) | ||
| for skill_name, errors in failures.items() | ||
| ) | ||
| session.error(f"Packaged skill validation failed:\n{details}") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,146 @@ | ||
| """Utilities for validating packaged agent skills.""" | ||
|
|
||
| from collections.abc import Mapping | ||
| from pathlib import Path | ||
| from typing import Final | ||
|
|
||
| import importlib_resources as resources | ||
| from importlib_resources.abc import Traversable | ||
|
|
||
| SKILLS_DIRECTORY: Final = "exasol.toolbox.skills" | ||
| PTB_SKILL_NAME: Final = "exasol-python-toolbox" | ||
| SKILL_FRONTMATTER_SEPARATOR: Final = "---" | ||
| SKILL_FORBIDDEN_TERMS: Final = ( | ||
| "main-branch", | ||
| "main branch", | ||
| "master-branch", | ||
| "master branch", | ||
| "inventory", | ||
| "source-map", | ||
| ) | ||
| SKILL_FILES: Final = ("SKILL.md",) | ||
|
|
||
|
|
||
| def get_skill_path(skill_name: str = PTB_SKILL_NAME) -> Path: | ||
| def get_skill_path(skill_name: str = PTB_SKILL_NAME) -> Traversable: | ||
| """ | ||
| Return the path to a packaged skill. | ||
| """ | ||
| return Path(str(resources.files(SKILLS_DIRECTORY) / skill_name)) | ||
| return resources.files(SKILLS_DIRECTORY) / skill_name | ||
|
|
||
|
|
||
| def _find_files( | ||
| root: Traversable, relative_directory: str = "" | ||
| ) -> dict[str, Traversable]: | ||
| """Return all files below a package resource directory.""" | ||
| files: dict[str, Traversable] = {} | ||
| for child in root.iterdir(): | ||
| relative_path = f"{relative_directory}{child.name}" | ||
| if child.is_file(): | ||
| files[relative_path] = child | ||
| elif child.is_dir(): | ||
| files.update(_find_files(child, f"{relative_path}/")) | ||
| return files | ||
|
|
||
|
|
||
| def get_skill_files(skill_name: str = PTB_SKILL_NAME) -> Mapping[str, Path]: | ||
| def get_skill_files(skill_name: str = PTB_SKILL_NAME) -> Mapping[str, Traversable]: | ||
| """ | ||
| Return packaged skill files. | ||
|
|
||
| The keys are paths relative to the skill root. | ||
| """ | ||
| skill_path = get_skill_path(skill_name) | ||
| return { | ||
| str(path.relative_to(skill_path)): path | ||
| for path in skill_path.rglob("*") | ||
| if path.is_file() | ||
| } | ||
| return _find_files(get_skill_path(skill_name)) | ||
|
|
||
|
|
||
| def get_packaged_skill_names() -> tuple[str, ...]: | ||
| """Return the names of all skills packaged with the toolbox.""" | ||
| return tuple( | ||
| sorted( | ||
| path.name | ||
| for path in resources.files(SKILLS_DIRECTORY).iterdir() | ||
| if path.is_dir() | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def _validate_frontmatter(content: str, skill_name: str) -> list[str]: | ||
| """Validate the frontmatter of a skill description.""" | ||
| parts = content.split(SKILL_FRONTMATTER_SEPARATOR, maxsplit=2) | ||
| if len(parts) != 3 or parts[0].strip(): | ||
| return ["SKILL.md must start with YAML frontmatter"] | ||
|
|
||
| errors = [] | ||
| frontmatter = parts[1] | ||
| if f"name: {skill_name}" not in frontmatter: | ||
| errors.append(f"frontmatter name must be {skill_name}") | ||
| if "description:" not in frontmatter: | ||
| errors.append("frontmatter must contain a description") | ||
| return errors | ||
|
|
||
|
|
||
| def _validate_markdown_file(relative_path: str, content: str) -> list[str]: | ||
| """Validate shared rules for one Markdown file.""" | ||
| errors: list[str] = [] | ||
| seen: dict[str, int] = {} | ||
| ignored_lines = {"---", "```bash", "```"} | ||
| for line_number, line in enumerate(content.splitlines(), 1): | ||
| normalized = line.strip().lower() | ||
| if not normalized or normalized in ignored_lines or normalized.startswith("|"): | ||
| continue | ||
| if normalized in seen: | ||
| errors.append( | ||
| f"{relative_path} duplicates line {seen[normalized]} " | ||
| f"at line {line_number}" | ||
| ) | ||
| seen[normalized] = line_number | ||
| return errors | ||
|
|
||
|
|
||
| def _validate_nox_syntax(relative_path: str, content: str) -> list[str]: | ||
| """Ensure Nox command syntax is kept in the dedicated reference.""" | ||
| nox_reference = "references/nox-sessions.md" | ||
| if relative_path != nox_reference and any( | ||
| command in content | ||
| for command in ("poetry run -- nox -s", "poetry run -- nox -l") | ||
| ): | ||
| return [f"{relative_path} contains Nox command syntax outside {nox_reference}"] | ||
| return [] | ||
|
|
||
|
|
||
| def validate_skill(skill_name: str) -> tuple[str, ...]: | ||
| """Return deterministic validation errors for a packaged skill. | ||
|
|
||
| The checks here are deliberately limited to properties shared by every PTB | ||
| skill. Assertions about a skill's specific content belong in that skill's | ||
| own tests. | ||
| """ | ||
| skill_files = get_skill_files(skill_name) | ||
| errors: list[str] = [] | ||
|
|
||
| for expected_file in SKILL_FILES: | ||
| if expected_file not in skill_files: | ||
| errors.append(f"missing required file: {expected_file}") | ||
|
|
||
| skill_content = skill_files.get("SKILL.md") | ||
| if skill_content is None: | ||
| return tuple(errors) | ||
|
|
||
| content = skill_content.read_text(encoding="utf-8") | ||
| errors.extend(_validate_frontmatter(content, skill_name)) | ||
|
|
||
| if "[TODO" in content: | ||
| errors.append("contains a TODO marker") | ||
|
|
||
| all_content = "\n".join( | ||
| path.read_text(encoding="utf-8") for path in skill_files.values() | ||
| ).lower() | ||
| for term in SKILL_FORBIDDEN_TERMS: | ||
| if term in all_content: | ||
| errors.append(f"contains forbidden term: {term}") | ||
|
|
||
| for relative_path, path in skill_files.items(): | ||
| if not relative_path.endswith(".md"): | ||
| continue | ||
| text = path.read_text(encoding="utf-8") | ||
| errors.extend(_validate_markdown_file(relative_path, text)) | ||
| errors.extend(_validate_nox_syntax(relative_path, text)) | ||
|
|
||
| return tuple(errors) |
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.
Could you try to improve readability a bit?
Maybe by adding a local method
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.
refactored