From a8d284f2bae4051316aa85ed9b1ad9208c985192 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 13 Jun 2026 20:56:23 +0200 Subject: [PATCH] Reject invalid skill paths --- src/skillspector/nodes/build_context.py | 33 ++++++------------- src/skillspector/nodes/resolve_input.py | 2 +- tests/integration/test_graph.py | 19 +++++++++-- tests/nodes/test_build_context.py | 43 ++++++++++++------------- 4 files changed, 47 insertions(+), 50 deletions(-) diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 694a048..ed32914 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -61,14 +61,17 @@ ) -def _resolve_skill_dir(state: SkillspectorState) -> Path | None: - """Resolve state skill_path to an existing directory Path, or None if missing/invalid.""" +def _resolve_skill_dir(state: SkillspectorState) -> Path: + """Resolve state skill_path to an existing directory Path.""" skill_path = state.get("skill_path") if not skill_path or not isinstance(skill_path, str) or not skill_path.strip(): - return None - resolved = Path(skill_path).resolve() + raise ValueError("skill_path is required; provide input_path or skill_path to scan") + try: + resolved = Path(skill_path).resolve() + except (OSError, RuntimeError) as e: + raise ValueError(f"Invalid skill_path: {skill_path}") from e if not resolved.is_dir(): - return None + raise ValueError(f"Invalid skill_path: {skill_path} is not an existing directory") return resolved @@ -212,32 +215,14 @@ def _parse_manifest(skill_dir: Path) -> dict[str, object]: return {} -def _minimal_update() -> dict[str, object]: - """Return minimal state update when skill_dir is missing or invalid.""" - return { - "components": [], - "file_cache": {}, - "ast_cache": {}, - "manifest": {}, - "previous_manifest": None, - "model_config": MODEL_CONFIG, - "component_metadata": [], - "has_executable_scripts": False, - } - - def build_context(state: SkillspectorState) -> dict[str, object]: """Build flat ScanContext fields from state skill_path (local directory). Resolves skill_path to a directory, walks files, builds file_cache and manifest. Returns only context keys; leaves findings untouched. - If skill_path is missing or not an existing directory, returns minimal - empty context (no exception). + Raises ValueError if skill_path is missing or not an existing directory. """ skill_dir = _resolve_skill_dir(state) - if skill_dir is None: - logger.debug("skill_path missing or not a directory; returning minimal context") - return _minimal_update() components = _walk_skill_files(skill_dir) file_cache = _read_file_cache(skill_dir, components) diff --git a/src/skillspector/nodes/resolve_input.py b/src/skillspector/nodes/resolve_input.py index 4a6d76d..a4e7555 100644 --- a/src/skillspector/nodes/resolve_input.py +++ b/src/skillspector/nodes/resolve_input.py @@ -38,7 +38,7 @@ def resolve_input(state: SkillspectorState) -> dict[str, object]: - If state has non-empty input_path: resolve it (Git URL, file URL, zip, file, or directory) and set skill_path. If resolution created a temp dir, set temp_dir_for_cleanup. - Else if state has skill_path: normalize to absolute path and set skill_path. - - Else: set skill_path to None (build_context will return minimal state). + - Else: set skill_path to None (build_context will raise a user-facing error). """ input_path = state.get("input_path") skill_path = state.get("skill_path") diff --git a/tests/integration/test_graph.py b/tests/integration/test_graph.py index 80b0896..031c7f9 100644 --- a/tests/integration/test_graph.py +++ b/tests/integration/test_graph.py @@ -18,6 +18,8 @@ import json from pathlib import Path +import pytest + from skillspector.graph import graph @@ -28,6 +30,7 @@ def test_graph_invoke_with_output_format_json(tmp_path: Path) -> None: { "skill_path": str(tmp_path), "output_format": "json", + "use_llm": False, } ) body = result.get("report_body", "") @@ -39,9 +42,9 @@ def test_graph_invoke_with_output_format_json(tmp_path: Path) -> None: assert "components" in data -def test_graph_invoke_returns_findings_and_report() -> None: +def test_graph_invoke_returns_findings_and_report(tmp_path: Path) -> None: """Graph runs to completion; returns findings, SARIF report, report_body, risk_score.""" - result = graph.invoke({"skill_path": "/tmp/dummy-skill"}) + result = graph.invoke({"skill_path": str(tmp_path), "use_llm": False}) assert "findings" in result assert isinstance(result["findings"], list) @@ -50,3 +53,15 @@ def test_graph_invoke_returns_findings_and_report() -> None: assert "report_body" in result assert result["risk_score"] >= 0 assert isinstance(result["report_body"], str) + + +def test_graph_invalid_skill_path_raises() -> None: + """Invalid skill_path raises instead of returning a clean low-risk report.""" + with pytest.raises(ValueError, match="not an existing directory"): + graph.invoke( + { + "skill_path": "/nonexistent/path/xyz", + "output_format": "json", + "use_llm": False, + } + ) diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index a0fc16b..26edee1 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -22,6 +22,8 @@ from pathlib import Path +import pytest + from skillspector.constants import MODEL_CONFIG from skillspector.nodes.build_context import build_context from skillspector.state import SkillspectorState @@ -89,48 +91,43 @@ def test_build_context_real_directory_with_skill_md(tmp_path: Path) -> None: def test_build_context_missing_skill_path() -> None: - """Missing skill_path returns minimal state (including model_config, component_metadata).""" + """Missing skill_path raises instead of producing a clean empty scan.""" state: SkillspectorState = {} - result = build_context(state) - assert result == { - "components": [], - "file_cache": {}, - "ast_cache": {}, - "manifest": {}, - "previous_manifest": None, - "model_config": MODEL_CONFIG, - "component_metadata": [], - "has_executable_scripts": False, - } + with pytest.raises(ValueError, match="skill_path is required"): + build_context(state) def test_build_context_empty_skill_path() -> None: - """Empty skill_path returns minimal state.""" + """Empty skill_path raises instead of producing a clean empty scan.""" state: SkillspectorState = {"skill_path": ""} - result = build_context(state) - assert result["components"] == [] - assert result["file_cache"] == {} - assert result["manifest"] == {} + with pytest.raises(ValueError, match="skill_path is required"): + build_context(state) def test_build_context_nonexistent_path() -> None: - """Non-existent path returns minimal state.""" + """Non-existent path raises instead of producing a clean empty scan.""" state: SkillspectorState = {"skill_path": "/nonexistent/path/xyz"} - result = build_context(state) - assert result["components"] == [] - assert result["file_cache"] == {} - assert result["manifest"] == {} + with pytest.raises(ValueError, match="not an existing directory"): + build_context(state) def test_build_context_path_is_file_not_dir(tmp_path: Path) -> None: - """Path that is a file (not directory) returns minimal state.""" + """Path that is a file raises instead of producing a clean empty scan.""" f = tmp_path / "file.txt" f.write_text("x", encoding="utf-8") state: SkillspectorState = {"skill_path": str(f)} + with pytest.raises(ValueError, match="not an existing directory"): + build_context(state) + + +def test_build_context_empty_directory_is_valid_empty_scan(tmp_path: Path) -> None: + """An existing empty directory is a valid scan target with no components.""" + state: SkillspectorState = {"skill_path": str(tmp_path)} result = build_context(state) assert result["components"] == [] assert result["file_cache"] == {} assert result["manifest"] == {} + assert result["model_config"] == MODEL_CONFIG def test_build_context_skips_skip_dirs(tmp_path: Path) -> None: