From 79871c20833059444a27f1e23cd7df056a389158 Mon Sep 17 00:00:00 2001 From: teknium1 Date: Wed, 4 Mar 2026 05:30:43 -0800 Subject: [PATCH] refactor: use Path.is_relative_to() for skill_view boundary check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the string-based startswith + os.sep approach with Path.is_relative_to() (Python 3.9+, we require 3.10+). This is the idiomatic pathlib way to check path containment — it handles separators, case sensitivity, and the equal-path case natively without string manipulation. Simplified tests to match: removed the now-unnecessary test_separator_is_os_native test since is_relative_to doesn't depend on separator choice. --- tests/tools/test_skill_view_path_check.py | 22 ++++++---------------- tools/skills_tool.py | 2 +- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/tests/tools/test_skill_view_path_check.py b/tests/tools/test_skill_view_path_check.py index fb91f8ba..07d3a3ab 100644 --- a/tests/tools/test_skill_view_path_check.py +++ b/tests/tools/test_skill_view_path_check.py @@ -1,29 +1,25 @@ -"""Tests for the skill_view path boundary check on all platforms. +"""Tests for the skill_view path boundary check. Regression test: the original check used a hardcoded "/" separator which fails on Windows where Path.resolve() returns backslash-separated paths. -The fix uses os.sep so the check works on both Unix and Windows. +Now uses Path.is_relative_to() which handles all platforms correctly. """ -import json import os import pytest from pathlib import Path def _path_escapes_skill_dir(resolved: Path, skill_dir_resolved: Path) -> bool: - """Reproduce the boundary check from tools/skills_tool.py (line 461). + """Reproduce the boundary check from tools/skills_tool.py. Returns True when the resolved path is OUTSIDE the skill directory. """ - return ( - not str(resolved).startswith(str(skill_dir_resolved) + os.sep) - and resolved != skill_dir_resolved - ) + return not resolved.is_relative_to(skill_dir_resolved) class TestSkillViewPathBoundaryCheck: - """Verify the os.sep fix prevents false positives on Windows.""" + """Verify the path boundary check works on all platforms.""" def test_valid_subpath_allowed(self, tmp_path): """A file inside the skill directory must NOT be flagged.""" @@ -82,7 +78,7 @@ class TestSkillViewPathBoundaryCheck: assert _path_escapes_skill_dir(resolved, skill_dir_resolved) is True def test_skill_dir_itself_allowed(self, tmp_path): - """Requesting the skill directory itself must be allowed (== check).""" + """Requesting the skill directory itself must be allowed.""" skill_dir = tmp_path / "skills" / "axolotl" skill_dir.mkdir(parents=True) @@ -91,12 +87,6 @@ class TestSkillViewPathBoundaryCheck: assert _path_escapes_skill_dir(resolved, skill_dir_resolved) is False - def test_separator_is_os_native(self): - """Confirm the check uses the platform's native separator.""" - # On Windows os.sep is '\\', on Unix '/'. - # The old bug hardcoded '/' which broke on Windows. - assert os.sep in ("\\", "/") - class TestOldCheckWouldFail: """Demonstrate the bug: the old hardcoded '/' check fails on Windows.""" diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 86aad973..31554943 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -458,7 +458,7 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str: try: resolved = target_file.resolve() skill_dir_resolved = skill_dir.resolve() - if not str(resolved).startswith(str(skill_dir_resolved) + os.sep) and resolved != skill_dir_resolved: + if not resolved.is_relative_to(skill_dir_resolved): return json.dumps({ "success": False, "error": "Path escapes skill directory boundary.",