From c6fcec376513afde056c1971a93ba4ddde74cca4 Mon Sep 17 00:00:00 2001 From: jana-selva Date: Wed, 30 Sep 2026 16:43:47 +0530 Subject: [PATCH 1/6] Refactor skill installation helpers --- doc/changes/unreleased.md | 2 +- exasol/toolbox/util/skills.py | 37 +++++++++++++++++++++++++---------- test/unit/util/skill_test.py | 33 +++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index 15304a8a9..bd93b52aa 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -3,7 +3,6 @@ ## Features * #940: Added shared validation for packaged agent skills and the `skills:check` Nox session. -* #938: Added the `skills:install` Nox session for installing the packaged PTB agent skill. * #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. @@ -12,3 +11,4 @@ * #924: Removed the deprecated `lint:dependencies` Nox session and its unused implementation and tests. * #972: Switched from ravsamhq/notify-slack-action to maintained slackapi/slack-github-action & removed workflow versioned header +* #967: Refactored skill installation into reusable filesystem helpers. diff --git a/exasol/toolbox/util/skills.py b/exasol/toolbox/util/skills.py index 6eae1ff0d..7a12414e3 100644 --- a/exasol/toolbox/util/skills.py +++ b/exasol/toolbox/util/skills.py @@ -71,19 +71,16 @@ 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 +94,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/unit/util/skill_test.py b/test/unit/util/skill_test.py index 0ccadbb28..4233fe55c 100644 --- a/test/unit/util/skill_test.py +++ b/test/unit/util/skill_test.py @@ -3,6 +3,11 @@ from exasol.toolbox.util import skills from exasol.toolbox.util.skill_eval import PackagedSkillEvalCases +from exasol.toolbox.util.skills import ( + _copy_skill_files, + _prepare_installation_directory, + _validate_skill_name, +) @pytest.fixture @@ -166,6 +171,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 _: {}) From 4aa30c6d3cd4319d67375f8a45da532ed36e0d28 Mon Sep 17 00:00:00 2001 From: jana-selva Date: Wed, 30 Sep 2026 16:49:15 +0530 Subject: [PATCH 2/6] format fix --- exasol/toolbox/util/skills.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/exasol/toolbox/util/skills.py b/exasol/toolbox/util/skills.py index 7a12414e3..febfc83ba 100644 --- a/exasol/toolbox/util/skills.py +++ b/exasol/toolbox/util/skills.py @@ -77,9 +77,7 @@ def _validate_skill_name(skill_name: str) -> None: raise ValueError(f"invalid skill name: {skill_name}") -def _prepare_installation_directory( - target_directory: Path, skill_name: str -) -> Path: +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): From 540436a347e81f2436200777a1f6577eac7eb569 Mon Sep 17 00:00:00 2001 From: jana-selva Date: Wed, 30 Sep 2026 16:50:38 +0530 Subject: [PATCH 3/6] Update skill installation changelog --- doc/changes/unreleased.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index bd93b52aa..02eb5224f 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -3,6 +3,7 @@ ## Features * #940: Added shared validation for packaged agent skills and the `skills:check` Nox session. +* #938: Added the `skills:install` Nox session for installing the packaged PTB agent skill. * #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. From 5fb5d3170a86f7d9342a42e4ee49b923586dfd40 Mon Sep 17 00:00:00 2001 From: jana-selva Date: Wed, 14 Oct 2026 16:32:11 +0530 Subject: [PATCH 4/6] Improve installation helper tests --- exasol/toolbox/util/skills.py | 6 +++-- test/unit/util/skill_test.py | 42 +++++++++-------------------------- 2 files changed, 15 insertions(+), 33 deletions(-) diff --git a/exasol/toolbox/util/skills.py b/exasol/toolbox/util/skills.py index febfc83ba..7de43beb6 100644 --- a/exasol/toolbox/util/skills.py +++ b/exasol/toolbox/util/skills.py @@ -95,10 +95,12 @@ def _prepare_installation_directory(target_directory: Path, skill_name: str) -> return target_skill -def _copy_skill_files(source_files: Mapping[str, Traversable], target: Path) -> None: +def _copy_skill_files( + source_files: Mapping[str, Traversable], target_directory: Path +) -> None: """Copy packaged skill files below an already validated target directory.""" for relative_path, source in source_files.items(): - destination = target / relative_path + destination = target_directory / relative_path destination.parent.mkdir(parents=True, exist_ok=True) destination.write_bytes(source.read_bytes()) diff --git a/test/unit/util/skill_test.py b/test/unit/util/skill_test.py index 4233fe55c..623b00343 100644 --- a/test/unit/util/skill_test.py +++ b/test/unit/util/skill_test.py @@ -10,21 +10,6 @@ ) -@pytest.fixture -def mock_packaged_skill_files(tmp_path, monkeypatch): - source = tmp_path / "source" - source.mkdir() - - skill_file = source / "SKILL.md" - skill_file.write_text("skill", encoding="utf-8") - - monkeypatch.setattr( - skills, - "get_skill_files", - lambda _: {"SKILL.md": skill_file}, - ) - - def test_validate_skill_accepts_packaged_ptb_skill(): assert skills.validate_skill(skills.PTB_SKILL_NAME) == () @@ -206,38 +191,33 @@ def test_install_skill_rejects_missing_skill(tmp_path, monkeypatch): skills.install_skill("example", tmp_path) -def test_install_skill_rejects_symlinked_target_directory( - tmp_path, mock_packaged_skill_files -): +def test_prepare_installation_directory_rejects_symlinked_target_directory(tmp_path): real_target = tmp_path / "real-target" real_target.mkdir() target_directory = tmp_path / "linked-target" target_directory.symlink_to(real_target, target_is_directory=True) with pytest.raises(ValueError, match="symlinked target directory"): - skills.install_skill("example", target_directory) + _prepare_installation_directory(target_directory, "example") -def test_install_skill_rejects_file_as_target(tmp_path, mock_packaged_skill_files): +def test_prepare_installation_directory_rejects_file_as_target(tmp_path): target_directory = tmp_path / ".agents" / "skills" target_directory.mkdir(parents=True) (target_directory / "example").write_text("not a directory", encoding="utf-8") with pytest.raises(ValueError, match="not a directory"): - skills.install_skill("example", target_directory) + _prepare_installation_directory(target_directory, "example") -def test_install_skill_rejects_symlink_target(tmp_path, monkeypatch): - source = tmp_path / "source" - source.mkdir() - skill_file = source / "SKILL.md" - skill_file.write_text("skill", encoding="utf-8") - monkeypatch.setattr(skills, "get_skill_files", lambda _: {"SKILL.md": skill_file}) +def test_prepare_installation_directory_rejects_symlink_target(tmp_path): target_directory = tmp_path / ".agents" / "skills" target_directory.mkdir(parents=True) - target = tmp_path / "elsewhere" - target.mkdir() - (target_directory / "example").symlink_to(target, target_is_directory=True) + symlink_target = tmp_path / "elsewhere" + symlink_target.mkdir() + (target_directory / "example").symlink_to( + symlink_target, target_is_directory=True + ) with pytest.raises(ValueError, match="refusing to replace symlink"): - skills.install_skill("example", target_directory) + _prepare_installation_directory(target_directory, "example") From 1737246bf38b972c41a750e8d5d82653a8e01108 Mon Sep 17 00:00:00 2001 From: jana-selva Date: Wed, 14 Oct 2026 16:38:38 +0530 Subject: [PATCH 5/6] Document installation safety checks --- exasol/toolbox/util/skills.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/exasol/toolbox/util/skills.py b/exasol/toolbox/util/skills.py index 7de43beb6..55ec3ce2e 100644 --- a/exasol/toolbox/util/skills.py +++ b/exasol/toolbox/util/skills.py @@ -73,6 +73,7 @@ def _has_symlink_in_parents(path: Path) -> bool: def _validate_skill_name(skill_name: str) -> None: """Reject skill names that could escape the skill installation directory.""" + # Validate before joining the name with the destination path. if Path(skill_name).name != skill_name: raise ValueError(f"invalid skill name: {skill_name}") @@ -80,6 +81,7 @@ def _validate_skill_name(skill_name: str) -> None: 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 + # Do not follow symlinks when creating or replacing an installation. if _has_symlink_in_parents(target_directory): raise ValueError( f"refusing to use symlinked target directory: {target_directory}" @@ -89,6 +91,7 @@ def _prepare_installation_directory(target_directory: Path, skill_name: str) -> if target_skill.exists() and not target_skill.is_dir(): raise ValueError(f"skill target is not a directory: {target_skill}") + # Replace an old installation only after validating its type and symlink state. if target_skill.exists(): shutil.rmtree(target_skill) target_skill.mkdir(parents=True, exist_ok=True) From 6ab791064b852e186955bcacc6fd3b51f4ec0298 Mon Sep 17 00:00:00 2001 From: jana-selva Date: Wed, 14 Oct 2026 16:59:27 +0530 Subject: [PATCH 6/6] Format helper tests --- test/unit/util/skill_test.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/unit/util/skill_test.py b/test/unit/util/skill_test.py index 623b00343..8716a661c 100644 --- a/test/unit/util/skill_test.py +++ b/test/unit/util/skill_test.py @@ -215,9 +215,7 @@ def test_prepare_installation_directory_rejects_symlink_target(tmp_path): target_directory.mkdir(parents=True) symlink_target = tmp_path / "elsewhere" symlink_target.mkdir() - (target_directory / "example").symlink_to( - symlink_target, target_is_directory=True - ) + (target_directory / "example").symlink_to(symlink_target, target_is_directory=True) with pytest.raises(ValueError, match="refusing to replace symlink"): _prepare_installation_directory(target_directory, "example")