diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index 7e971f989..e350a4dd2 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -7,3 +7,4 @@ * #942: Added api-contract-audit skill for identifying mismatches between type annotations, docstrings, and runtime behavior * #963: Extended packaged skill checks and installation to support multiple skills. +* #967: Refactored skill installation into reusable filesystem helpers. diff --git a/exasol/toolbox/util/skills.py b/exasol/toolbox/util/skills.py index 6eae1ff0d..febfc83ba 100644 --- a/exasol/toolbox/util/skills.py +++ b/exasol/toolbox/util/skills.py @@ -71,19 +71,14 @@ def _has_symlink_in_parents(path: Path) -> bool: return any(candidate.is_symlink() for candidate in (path, *path.parents)) -def install_skill( - skill_name: str = PTB_SKILL_NAME, - target_directory: Path | None = None, -) -> Path: - """Install a packaged skill into a project-local agent skill directory.""" +def _validate_skill_name(skill_name: str) -> None: + """Reject skill names that could escape the skill installation directory.""" if Path(skill_name).name != skill_name: raise ValueError(f"invalid skill name: {skill_name}") - source_files = get_skill_files(skill_name) - if not source_files: - raise ValueError(f"packaged skill does not exist: {skill_name}") - target_directory = target_directory or Path.cwd() / ".agents" / "skills" +def _prepare_installation_directory(target_directory: Path, skill_name: str) -> Path: + """Validate and recreate the destination directory for one skill.""" target_skill = target_directory / skill_name if _has_symlink_in_parents(target_directory): raise ValueError( @@ -97,10 +92,30 @@ def install_skill( if target_skill.exists(): shutil.rmtree(target_skill) target_skill.mkdir(parents=True, exist_ok=True) + return target_skill + + +def _copy_skill_files(source_files: Mapping[str, Traversable], target: Path) -> None: + """Copy packaged skill files below an already validated target directory.""" for relative_path, source in source_files.items(): - destination = target_skill / relative_path + destination = target / relative_path destination.parent.mkdir(parents=True, exist_ok=True) destination.write_bytes(source.read_bytes()) + + +def install_skill( + skill_name: str = PTB_SKILL_NAME, + target_directory: Path | None = None, +) -> Path: + """Install a packaged skill into a project-local agent skill directory.""" + _validate_skill_name(skill_name) + source_files = get_skill_files(skill_name) + if not source_files: + raise ValueError(f"packaged skill does not exist: {skill_name}") + + target_directory = target_directory or Path.cwd() / ".agents" / "skills" + target_skill = _prepare_installation_directory(target_directory, skill_name) + _copy_skill_files(source_files, target_skill) return target_skill diff --git a/test/integration/project-template/nox_test.py b/test/integration/project-template/nox_test.py index 9cfd87927..f42751e5a 100644 --- a/test/integration/project-template/nox_test.py +++ b/test/integration/project-template/nox_test.py @@ -4,6 +4,43 @@ "api-contract-audit": 1, "exasol-python-toolbox": 5, } +EXPECTED_NOX_SESSIONS = { + "format:fix", + "format:check", + "project:check", + "test:unit", + "test:integration", + "test:coverage", + "lint:code", + "lint:typing", + "lint:security", + "lint:dependencies", + "docs:multiversion", + "docs:build", + "docs:open", + "docs:clean", + "links:list", + "links:check", + "changelog:updated", + "release:prepare", + "release:update", + "release:trigger", + "skills:check", + "skills:install", + "matrix:generate", + "artifacts:validate", + "artifacts:copy", + "sonar:check", + "dependency:licenses", + "dependency:audit", + "vulnerabilities:update", + "vulnerabilities:resolved", + "dependency:sbom", + "package:check", + "workflow:check", + "workflow:generate", + "workflow:audit", +} class TestSpecificNoxTasks: @@ -113,3 +150,13 @@ def test_skills_install_and_check(self, poetry_path, run_command, new_project): output = run_command(skills_check) assert output.returncode == 0 + + def test_exposed_nox_sessions(self, poetry_path, run_command): + output = run_command([poetry_path, "run", "--", "nox", "-l"]) + sessions = { + line[2:].split(" ->", maxsplit=1)[0] + for line in output.stdout.splitlines() + if line.startswith(("* ", "- ")) + } + + assert sessions == EXPECTED_NOX_SESSIONS diff --git a/test/unit/nox/_documentation_test.py b/test/unit/nox/_documentation_test.py index 85d0a2d37..c7b8261cf 100644 --- a/test/unit/nox/_documentation_test.py +++ b/test/unit/nox/_documentation_test.py @@ -1,7 +1,6 @@ import shutil from unittest.mock import ( MagicMock, - Mock, patch, ) @@ -9,18 +8,10 @@ from nox.sessions import _SessionQuit from exasol.toolbox.nox._documentation import ( - _build_docs, - _build_multiversion_docs, _docs_links_check, - _docs_list_links, - build_docs, - build_multiversion, - clean_docs, docs_links_check, docs_list_links, - open_docs, ) -from exasol.toolbox.nox._shared import DOCS_OUTPUT_DIR from noxconfig import PROJECT_CONFIG @@ -81,87 +72,6 @@ def test_raises_error_for_rcode_not_0(nox_session, config): docs_list_links(nox_session) -def test_build_docs_runs_sphinx(nox_session, config): - with patch("exasol.toolbox.nox._documentation._build_docs") as build: - with patch("exasol.toolbox.nox._documentation.PROJECT_CONFIG", new=config): - build_docs(nox_session) - - build.assert_called_once_with(nox_session, config) - - -def test_build_multiversion_docs_runs_sphinx(nox_session, config): - with patch("exasol.toolbox.nox._documentation._build_multiversion_docs") as build: - with patch("exasol.toolbox.nox._documentation.PROJECT_CONFIG", new=config): - build_multiversion(nox_session) - - build.assert_called_once_with(nox_session, config) - - -def test_build_docs_command(config): - session = Mock() - _build_docs(session, config) - - session.run.assert_called_once_with( - "sphinx-build", - "-W", - "-b", - "html", - f"{config.documentation_path}", - DOCS_OUTPUT_DIR, - ) - - -def test_build_multiversion_docs_commands(config): - session = Mock() - _build_multiversion_docs(session, config) - - assert session.run.call_count == 2 - session.run.assert_any_call( - "sphinx-multiversion", - f"{config.documentation_path}", - DOCS_OUTPUT_DIR, - ) - session.run.assert_any_call("touch", f"{DOCS_OUTPUT_DIR}/.nojekyll") - - -def test_open_docs_reports_missing_output(nox_session, config): - with patch("exasol.toolbox.nox._documentation.PROJECT_CONFIG", new=config): - with pytest.raises(_SessionQuit): - open_docs(nox_session) - - -def test_open_docs_opens_index(nox_session, config): - docs_folder = config.root_path / DOCS_OUTPUT_DIR - docs_folder.mkdir() - (docs_folder / "index.html").touch() - with patch("exasol.toolbox.nox._documentation.PROJECT_CONFIG", new=config): - with patch( - "exasol.toolbox.nox._documentation.webbrowser.open_new_tab" - ) as open_tab: - open_docs(nox_session) - - open_tab.assert_called_once_with((docs_folder / "index.html").as_uri()) - - -def test_clean_docs_removes_output(nox_session, config): - docs_folder = config.root_path / DOCS_OUTPUT_DIR - docs_folder.mkdir() - with patch("exasol.toolbox.nox._documentation.PROJECT_CONFIG", new=config): - clean_docs(nox_session) - - assert not docs_folder.exists() - - -def test_docs_list_links_reports_sphinx_failure(tmp_path): - with patch( - "exasol.toolbox.nox._documentation.subprocess.run", - return_value=MagicMock(returncode=2, stderr="sphinx failed"), - ): - result = _docs_list_links(tmp_path) - - assert result == (2, "sphinx failed") - - @pytest.mark.slow @pytest.mark.parametrize( "file_content, expected_code, expected_message", diff --git a/test/unit/nox/_test_test.py b/test/unit/nox/_test_test.py index 341704de2..c44a347cd 100644 --- a/test/unit/nox/_test_test.py +++ b/test/unit/nox/_test_test.py @@ -1,13 +1,9 @@ import shutil -from unittest.mock import ( - Mock, - patch, -) +from unittest.mock import patch import pytest from exasol.toolbox.nox._test import ( - _coverage, _test_command, coverage, integration_tests, @@ -140,22 +136,3 @@ def test_coverage_uses_integration_test_context(nox_session): PROJECT_CONFIG, {"coverage": True, "db_version": "8.29.13", "fwd-args": []}, ) - - -def test_coverage_runs_tests_and_reports(tmp_path, test_project_config_factory): - config = test_project_config_factory(root_path=tmp_path) - coverage_file = tmp_path / ".coverage" - coverage_file.touch() - context = {"coverage": True, "db_version": "8.29.13", "fwd-args": []} - session = Mock() - - with ( - patch("exasol.toolbox.nox._test._unit_tests") as unit, - patch("exasol.toolbox.nox._test._integration_tests") as integration, - ): - _coverage(session, config, context) - - assert not coverage_file.exists() - unit.assert_called_once_with(session, config, context) - integration.assert_called_once_with(session, config, context) - session.run.assert_called_once_with("coverage", "report", "-m") diff --git a/test/unit/nox/tasks_test.py b/test/unit/nox/tasks_test.py deleted file mode 100644 index 662f2c677..000000000 --- a/test/unit/nox/tasks_test.py +++ /dev/null @@ -1,27 +0,0 @@ -from unittest.mock import ( - Mock, - patch, -) - -from exasol.toolbox.nox import tasks - - -def test_check_runs_all_project_checks(): - session = Mock() - context = {"coverage": True, "db_version": "8.29.13", "fwd-args": []} - files = ("example.py",) - - with ( - patch.object(tasks, "_integration_test_context", return_value=context), - patch.object(tasks, "get_filtered_python_files", return_value=files), - patch.object(tasks, "_code_format") as code_format, - patch.object(tasks, "_pylint") as pylint, - patch.object(tasks, "_type_check") as type_check, - patch.object(tasks, "_coverage") as coverage, - ): - tasks.check(session) - - code_format.assert_called_once_with(session, tasks.Mode.Check, files) - pylint.assert_called_once_with(session, files) - type_check.assert_called_once_with(session, files) - coverage.assert_called_once_with(session, tasks.PROJECT_CONFIG, context) diff --git a/test/unit/skills_test.py b/test/unit/skills_test.py index 91a9628d0..3e01bad33 100644 --- a/test/unit/skills_test.py +++ b/test/unit/skills_test.py @@ -1,4 +1,3 @@ -from collections.abc import Mapping from pathlib import Path from subprocess import run from zipfile import ZipFile @@ -44,52 +43,6 @@ def _skills_with_eval_cases() -> list[str]: ] -def _validate_eval_cases(eval_cases: object, skill_name: str) -> list[str]: - # Eval cases are test resources, so validate their reusable schema here - # instead of coupling production skill discovery to test-only files. - errors: list[str] = [] - if not isinstance(eval_cases, Mapping): - return ["evaluation cases must be a mapping"] - if eval_cases.get("version") != 1: - errors.append("version must be 1") - if eval_cases.get("skill") != skill_name: - errors.append(f"skill must be {skill_name}") - - cases = eval_cases.get("cases") - if not isinstance(cases, list) or not cases: - return errors + ["cases must be a non-empty list"] - - ids: list[str] = [] - for index, case in enumerate(cases): - if not isinstance(case, Mapping): - errors.append(f"case {index} must be a mapping") - continue - case_id = case.get("id") - if not isinstance(case_id, str) or not case_id.strip(): - errors.append(f"case {index} must have a non-empty id") - else: - ids.append(case_id) - for field in ("category", "prompt"): - value = case.get(field) - if not isinstance(value, str) or not value.strip(): - errors.append(f"case {index} must have a non-empty {field}") - - expected = case.get("expected") - if not isinstance(expected, Mapping): - errors.append(f"case {index} expected must be a mapping") - continue - for field in ("must_include", "must_not_include"): - values = expected.get(field) - if not isinstance(values, list) or not values: - errors.append(f"case {index} {field} must be a non-empty list") - elif not all(isinstance(value, str) and value.strip() for value in values): - errors.append(f"case {index} {field} must contain non-empty strings") - - if len(ids) != len(set(ids)): - errors.append("case ids must be unique") - return errors - - def test_ptb_skill_resources_are_available(): skill_files = get_skill_files(PTB_SKILL_NAME) @@ -157,87 +110,36 @@ def test_ptb_skill_frontmatter_is_complete(): @pytest.mark.parametrize("skill_name", _skills_with_eval_cases()) class TestPackagedSkillEvalCases: - def test_schema_is_valid(self, skill_name): - eval_cases = _load_eval_cases(skill_name) - - assert _validate_eval_cases(eval_cases, skill_name) == [] - - -def _minimal_eval_cases() -> dict: - return { - "version": 1, - "skill": "example", - "cases": [ - { - "id": "case", - "category": "quality", - "prompt": "Check the API.", - "expected": { - "must_include": ["finding"], - "must_not_include": ["fix"], - }, - } - ], - } - - -class TestEvalCaseValidation: - @staticmethod - def _assert_rejected(change, expected_error): - eval_cases = _minimal_eval_cases() - # Each mutation represents a malformed future eval_cases.yml file. - change(eval_cases) - - assert expected_error in _validate_eval_cases(eval_cases, "example") - - def test_rejects_invalid_version(self): - self._assert_rejected(lambda data: data.update(version=2), "version must be 1") - - def test_rejects_invalid_skill_name(self): - self._assert_rejected( - lambda data: data.update(skill="other"), "skill must be example" - ) - - def test_rejects_empty_cases(self): - self._assert_rejected( - lambda data: data["cases"].clear(), "cases must be a non-empty list" - ) - - def test_rejects_duplicate_case_ids(self): - self._assert_rejected( - lambda data: data["cases"].append(data["cases"][0].copy()), - "case ids must be unique", - ) - - def test_rejects_empty_category(self): - self._assert_rejected( - lambda data: data["cases"][0].update(category=""), - "case 0 must have a non-empty category", - ) - - def test_rejects_empty_prompt(self): - self._assert_rejected( - lambda data: data["cases"][0].update(prompt=""), - "case 0 must have a non-empty prompt", - ) - - def test_rejects_missing_expected_mapping(self): - self._assert_rejected( - lambda data: data["cases"][0].update(expected=None), - "case 0 expected must be a mapping", - ) - - def test_rejects_empty_must_include(self): - self._assert_rejected( - lambda data: data["cases"][0]["expected"].update(must_include=[]), - "case 0 must_include must be a non-empty list", - ) - - def test_rejects_blank_must_not_include(self): - self._assert_rejected( - lambda data: data["cases"][0]["expected"].update(must_not_include=[""]), - "case 0 must_not_include must contain non-empty strings", - ) + @pytest.fixture + def eval_cases(self, skill_name): + # Load the packaged artifact once so all checks inspect the same data. + return _load_eval_cases(skill_name) + + def test_has_expected_metadata(self, eval_cases, skill_name): + + assert eval_cases["version"] == 1 + assert eval_cases["skill"] == skill_name + assert isinstance(eval_cases["cases"], list) + assert eval_cases["cases"] + + def test_cases_have_required_fields(self, eval_cases): + for case in eval_cases["cases"]: + assert case["id"].strip() + assert case["category"].strip() + assert case["prompt"].strip() + + def test_cases_have_response_constraints(self, eval_cases): + for case in eval_cases["cases"]: + expected = case["expected"] + assert expected["must_include"] + assert expected["must_not_include"] + assert all(value.strip() for value in expected["must_include"]) + assert all(value.strip() for value in expected["must_not_include"]) + + def test_case_ids_are_unique(self, eval_cases): + ids = [case["id"] for case in eval_cases["cases"]] + + assert len(ids) == len(set(ids)) def test_ptb_skill_eval_cases_cover_ticket_scope(): diff --git a/test/unit/util/skill_test.py b/test/unit/util/skill_test.py index d96f254ba..c6c0b761e 100644 --- a/test/unit/util/skill_test.py +++ b/test/unit/util/skill_test.py @@ -1,6 +1,11 @@ import pytest from exasol.toolbox.util import skills +from exasol.toolbox.util.skills import ( + _copy_skill_files, + _prepare_installation_directory, + _validate_skill_name, +) def test_validate_skill_accepts_packaged_ptb_skill(): @@ -115,6 +120,34 @@ def test_install_skill_rejects_path_traversal(tmp_path): skills.install_skill("../outside", tmp_path) +def test_validate_skill_name_rejects_path_traversal(): + with pytest.raises(ValueError, match="invalid skill name"): + _validate_skill_name("nested/example") + + +def test_prepare_installation_directory_replaces_existing_directory(tmp_path): + target_directory = tmp_path / ".agents" / "skills" + target_skill = target_directory / "example" + target_skill.mkdir(parents=True) + (target_skill / "stale.md").write_text("stale", encoding="utf-8") + + prepared = _prepare_installation_directory(target_directory, "example") + + assert prepared == target_skill + assert not (target_skill / "stale.md").exists() + + +def test_copy_skill_files_copies_nested_files(tmp_path): + source = tmp_path / "source.md" + source.write_text("content", encoding="utf-8") + target = tmp_path / "target" + target.mkdir() + + _copy_skill_files({"references/source.md": source}, target) + + assert (target / "references/source.md").read_text(encoding="utf-8") == "content" + + def test_install_skill_rejects_missing_skill(tmp_path, monkeypatch): monkeypatch.setattr(skills, "get_skill_files", lambda _: {})