diff --git a/src/napoln/commands/add.py b/src/napoln/commands/add.py index 91cb06c..db52286 100644 --- a/src/napoln/commands/add.py +++ b/src/napoln/commands/add.py @@ -20,7 +20,13 @@ resolve_git, resolve_local, ) -from napoln.errors import MultipleSkillsError, ResolverError +from napoln.errors import ( + ManifestError, + MultipleSkillsError, + PlacementError, + ResolverError, + StoreError, +) from napoln.prompts import SkillChoice, pick_skills @@ -297,7 +303,7 @@ def _install_single_skill( store_path, content_hash = store.store_skill( resolved.skill_dir, install_id, version, napoln_home ) - except Exception as e: + except (StoreError, OSError) as e: output.error(f"Failed to store skill '{install_id}': {e}") return 1 @@ -320,7 +326,7 @@ def _install_single_skill( link_mode=link_mode, scope=scope, ) - except Exception as e: + except (PlacementError, OSError) as e: output.error(f"Failed to place '{install_id}' for {path_agents[0].display_name}: {e}") return 1 @@ -363,7 +369,7 @@ def _pick_from_multi_skill_repo( continue try: mf = manifest.read_manifest(mf_path) - except Exception: + except (ManifestError, OSError): continue for entry in mf.skills.values(): installed_sources.add(entry.source) diff --git a/src/napoln/commands/enable.py b/src/napoln/commands/enable.py index b5a500b..93aac60 100644 --- a/src/napoln/commands/enable.py +++ b/src/napoln/commands/enable.py @@ -9,6 +9,7 @@ from napoln.core import agents as agents_mod from napoln.core import linker, manifest, store from napoln.core.home import get_napoln_home +from napoln.errors import PlacementError, StoreError from napoln.prompts import SkillChoice, pick_agents, pick_skills @@ -64,7 +65,7 @@ def _place_skill_for_agent( skill_entry.source, napoln_home, ) - except Exception as e: + except (StoreError, OSError) as e: output.error(f"Failed to retrieve '{skill_name}' from store: {e}") return None @@ -82,7 +83,7 @@ def _place_skill_for_agent( link_mode, ) return link_mode - except Exception as e: + except (PlacementError, OSError) as e: output.error(f"Failed to place '{skill_name}': {e}") return None diff --git a/src/napoln/commands/install.py b/src/napoln/commands/install.py index 8e30722..0aec3cb 100644 --- a/src/napoln/commands/install.py +++ b/src/napoln/commands/install.py @@ -7,7 +7,7 @@ from napoln import output from napoln.core import linker, manifest, store from napoln.core.home import NAPOLN_DIR, get_napoln_home -from napoln.errors import NapolnError +from napoln.errors import NapolnError, PlacementError def _sync_manifest( @@ -51,7 +51,7 @@ def _sync_manifest( if result is not None: output.success(f"Restored '{skill_name}' to {placement_path}") synced += 1 - except Exception as e: + except (PlacementError, OSError) as e: output.error(f"Failed to restore '{skill_name}' to {placement_path}: {e}") errors += 1 diff --git a/src/napoln/core/manifest.py b/src/napoln/core/manifest.py index 52311d7..1d428cf 100644 --- a/src/napoln/core/manifest.py +++ b/src/napoln/core/manifest.py @@ -81,7 +81,7 @@ def read_manifest(path: Path) -> Manifest: try: data = tomllib.loads(path.read_text(encoding="utf-8")) - except Exception as e: + except (OSError, tomllib.TOMLDecodeError) as e: raise ManifestError( f"Could not read manifest: {path}", cause=str(e), @@ -159,13 +159,12 @@ def write_manifest(manifest: Manifest, path: Path) -> None: try: tmp_path.write_text(serialized, encoding="utf-8") os.replace(tmp_path, path) - except Exception: + finally: if tmp_path.exists(): try: tmp_path.unlink() except OSError: pass - raise def add_skill_to_manifest( diff --git a/src/napoln/core/merger.py b/src/napoln/core/merger.py index e549ce4..d404361 100644 --- a/src/napoln/core/merger.py +++ b/src/napoln/core/merger.py @@ -229,5 +229,5 @@ def has_conflict_markers(file_path: Path) -> bool: try: content = file_path.read_text(encoding="utf-8") return "<<<<<<< " in content and "=======" in content and ">>>>>>> " in content - except Exception: + except (OSError, UnicodeDecodeError): return False diff --git a/src/napoln/core/resolver.py b/src/napoln/core/resolver.py index f6cff2b..b14e823 100644 --- a/src/napoln/core/resolver.py +++ b/src/napoln/core/resolver.py @@ -525,7 +525,7 @@ def _extract_version(skill_dir: Path) -> str: return str(metadata["version"]) if "version" in frontmatter: return str(frontmatter["version"]) - except Exception: + except (OSError, yaml.YAMLError, UnicodeDecodeError): pass return DEFAULT_VERSION @@ -549,7 +549,7 @@ def _extract_description(skill_dir: Path) -> str: frontmatter = yaml.safe_load(content[3:end]) if isinstance(frontmatter, dict): return str(frontmatter.get("description", "")) - except Exception: + except (OSError, yaml.YAMLError, UnicodeDecodeError): pass return "" diff --git a/src/napoln/core/store.py b/src/napoln/core/store.py index 73bd5e4..d6474c9 100644 --- a/src/napoln/core/store.py +++ b/src/napoln/core/store.py @@ -71,7 +71,7 @@ def store_skill( # Atomic rename temp_path.rename(store_path) - except Exception: + except OSError: if temp_path.exists(): shutil.rmtree(temp_path) raise diff --git a/tests/unit/test_add_logic.py b/tests/unit/test_add_logic.py index af7f4fe..d484ba9 100644 --- a/tests/unit/test_add_logic.py +++ b/tests/unit/test_add_logic.py @@ -3,8 +3,19 @@ from __future__ import annotations import tomllib +from pathlib import Path +from unittest.mock import MagicMock -from napoln.commands.add import _ensure_initialized +import pytest + +from napoln.commands.add import ( + _ensure_initialized, + _install_single_skill, + _pick_from_multi_skill_repo, +) +from napoln.core import manifest as manifest_mod +from napoln.core.resolver import ResolvedSource, SourceType +from napoln.errors import ManifestError, MultipleSkillsError, PlacementError, StoreError class TestEnsureInitialized: @@ -45,3 +56,206 @@ def test_creates_nested_parent_dirs(self, tmp_path): assert home.is_dir() assert (home / "store").is_dir() + + +class TestInstallSingleSkillExceptions: + """Regression: broad except Exception swallows programming bugs.""" + + @pytest.fixture + def resolved(self, tmp_path): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + '---\nname: test-skill\ndescription: x\nmetadata:\n version: "1.0.0"\n---\n# Hello' + ) + return ResolvedSource( + source_type=SourceType.LOCAL, + source_id=str(skill_dir), + skill_dir=skill_dir, + version="1.0.0", + ) + + @pytest.fixture + def agent_config(self): + cfg = MagicMock() + cfg.id = "claude-code" + cfg.display_name = "Claude Code" + return cfg + + @pytest.fixture + def manifest_and_path(self, tmp_path): + path = tmp_path / "manifest.toml" + mf = manifest_mod.Manifest() + return mf, path + + def test_store_error_outputs_and_returns_1( + self, resolved, agent_config, manifest_and_path, monkeypatch + ): + mf, path = manifest_and_path + errors = [] + + def capture_error(msg, **_kwargs): + errors.append(msg) + + monkeypatch.setattr("napoln.commands.add.output.error", capture_error) + monkeypatch.setattr( + "napoln.commands.add.store.store_skill", + lambda *_a, **_k: (_ for _ in ()).throw(StoreError("store failed")), + ) + + code = _install_single_skill( + resolved, + "test-skill", + [agent_config], + Path.home(), + Path.home(), + "global", + None, + mf, + path, + False, + ) + assert code == 1 + assert any("Failed to store skill" in e for e in errors) + + def test_store_type_error_propagates( + self, resolved, agent_config, manifest_and_path, monkeypatch + ): + """Programming bugs must not be swallowed.""" + mf, path = manifest_and_path + + monkeypatch.setattr( + "napoln.commands.add.store.store_skill", + lambda *_a, **_k: (_ for _ in ()).throw(TypeError("programming bug")), + ) + + with pytest.raises(TypeError, match="programming bug"): + _install_single_skill( + resolved, + "test-skill", + [agent_config], + Path.home(), + Path.home(), + "global", + None, + mf, + path, + False, + ) + + def test_placement_error_outputs_and_returns_1( + self, resolved, agent_config, manifest_and_path, monkeypatch, tmp_path + ): + mf, path = manifest_and_path + errors = [] + + def capture_error(msg, **_kwargs): + errors.append(msg) + + monkeypatch.setattr("napoln.commands.add.output.error", capture_error) + monkeypatch.setattr( + "napoln.commands.add.store.store_skill", + lambda *_a, **_k: (tmp_path / "store", "hash123"), + ) + monkeypatch.setattr( + "napoln.commands.add.linker.place_skill", + lambda *_a, **_k: (_ for _ in ()).throw(PlacementError("placement failed")), + ) + + code = _install_single_skill( + resolved, + "test-skill", + [agent_config], + Path.home(), + Path.home(), + "global", + None, + mf, + path, + False, + ) + assert code == 1 + assert any("Failed to place" in e for e in errors) + + def test_placement_type_error_propagates( + self, resolved, agent_config, manifest_and_path, monkeypatch, tmp_path + ): + """Programming bugs must not be swallowed.""" + mf, path = manifest_and_path + + monkeypatch.setattr( + "napoln.commands.add.store.store_skill", + lambda *_a, **_k: (tmp_path / "store", "hash123"), + ) + monkeypatch.setattr( + "napoln.commands.add.linker.place_skill", + lambda *_a, **_k: (_ for _ in ()).throw(TypeError("programming bug")), + ) + + with pytest.raises(TypeError, match="programming bug"): + _install_single_skill( + resolved, + "test-skill", + [agent_config], + Path.home(), + Path.home(), + "global", + None, + mf, + path, + False, + ) + + +class TestPickFromMultiSkillRepoExceptions: + """Regression: broad except Exception swallows programming bugs.""" + + def test_manifest_error_continues(self, tmp_path, monkeypatch): + repo_dir = tmp_path / "repo" + repo_dir.mkdir() + skill_dir = repo_dir / "skill-a" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: skill-a\n---\n") + + err = MultipleSkillsError(repo_dir, [skill_dir]) + parsed = MagicMock() + parsed.host = "github.com" + parsed.owner = "owner" + parsed.repo = "repo" + parsed.version = "" + + monkeypatch.setattr( + "napoln.commands.add.manifest.read_manifest", + lambda *_a, **_k: (_ for _ in ()).throw(ManifestError("bad manifest")), + ) + monkeypatch.setattr("napoln.commands.add.pick_skills", lambda _choices: []) + + result = _pick_from_multi_skill_repo(err, parsed, "owner/repo", None, tmp_path / ".napoln") + assert result is None + + def test_manifest_type_error_propagates(self, tmp_path, monkeypatch): + """Programming bugs must not be swallowed.""" + repo_dir = tmp_path / "repo" + repo_dir.mkdir() + skill_dir = repo_dir / "skill-a" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: skill-a\n---\n") + + err = MultipleSkillsError(repo_dir, [skill_dir]) + parsed = MagicMock() + parsed.host = "github.com" + parsed.owner = "owner" + parsed.repo = "repo" + parsed.version = "" + + napoln_home = tmp_path / ".napoln" + napoln_home.mkdir() + (napoln_home / "manifest.toml").write_text("[napoln]\nschema = 1\n", encoding="utf-8") + + monkeypatch.setattr( + "napoln.commands.add.manifest.read_manifest", + lambda *_a, **_k: (_ for _ in ()).throw(TypeError("programming bug")), + ) + + with pytest.raises(TypeError, match="programming bug"): + _pick_from_multi_skill_repo(err, parsed, "owner/repo", None, napoln_home) diff --git a/tests/unit/test_enable.py b/tests/unit/test_enable.py index 31f88a9..8614ed2 100644 --- a/tests/unit/test_enable.py +++ b/tests/unit/test_enable.py @@ -2,8 +2,14 @@ from __future__ import annotations -from napoln.commands.enable import _get_skills_to_enable +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from napoln.commands.enable import _get_skills_to_enable, _place_skill_for_agent from napoln.core import manifest as manifest_mod +from napoln.errors import PlacementError, StoreError class TestGetSkillsToEnable: @@ -115,3 +121,121 @@ def test_skill_placed_for_different_agent(self): result = _get_skills_to_enable(mf, "hermes") assert len(result) == 1 assert result[0][0] == "skill-a" + + +class TestPlaceSkillForAgentExceptions: + """Regression: broad except Exception swallows programming bugs.""" + + @pytest.fixture + def skill_entry(self): + return manifest_mod.SkillEntry( + source="owner/repo", + version="1.0.0", + store_hash="abc123", + installed="2024-01-01T00:00:00Z", + updated="2024-01-01T00:00:00Z", + ) + + @pytest.fixture + def agent_config(self): + cfg = MagicMock() + cfg.id = "claude-code" + cfg.display_name = "Claude Code" + cfg.skill_path = lambda home, name, scope, project_root: Path(home) / "skills" / name + return cfg + + def test_store_error_outputs_and_returns_none(self, skill_entry, agent_config, monkeypatch): + errors = [] + + def capture_error(msg, **_kwargs): + errors.append(msg) + + monkeypatch.setattr("napoln.commands.enable.output.error", capture_error) + monkeypatch.setattr( + "napoln.commands.enable.store.ensure_stored", + lambda *_a, **_k: (_ for _ in ()).throw(StoreError("store failed")), + ) + + result = _place_skill_for_agent( + "my-skill", + skill_entry, + agent_config, + Path("/.napoln"), + Path.home(), + "global", + None, + ) + assert result is None + assert any("Failed to retrieve" in e for e in errors) + + def test_store_type_error_propagates(self, skill_entry, agent_config, monkeypatch): + """Programming bugs must not be swallowed.""" + monkeypatch.setattr( + "napoln.commands.enable.store.ensure_stored", + lambda *_a, **_k: (_ for _ in ()).throw(TypeError("programming bug")), + ) + + with pytest.raises(TypeError, match="programming bug"): + _place_skill_for_agent( + "my-skill", + skill_entry, + agent_config, + Path("/.napoln"), + Path.home(), + "global", + None, + ) + + def test_placement_error_outputs_and_returns_none( + self, skill_entry, agent_config, monkeypatch, tmp_path + ): + errors = [] + + def capture_error(msg, **_kwargs): + errors.append(msg) + + monkeypatch.setattr("napoln.commands.enable.output.error", capture_error) + monkeypatch.setattr( + "napoln.commands.enable.store.ensure_stored", + lambda *_a, **_k: tmp_path / "store", + ) + monkeypatch.setattr( + "napoln.commands.enable.linker.place_skill", + lambda *_a, **_k: (_ for _ in ()).throw(PlacementError("placement failed")), + ) + + result = _place_skill_for_agent( + "my-skill", + skill_entry, + agent_config, + Path("/.napoln"), + Path.home(), + "global", + None, + ) + assert result is None + assert any("Failed to place" in e for e in errors) + + def test_placement_type_error_propagates( + self, skill_entry, agent_config, monkeypatch, tmp_path + ): + """Programming bugs must not be swallowed.""" + monkeypatch.setattr( + "napoln.commands.enable.store.ensure_stored", + lambda *_a, **_k: tmp_path / "store", + ) + monkeypatch.setattr( + "napoln.commands.enable.linker.place_skill", + lambda *_a, **_k: (_ for _ in ()).throw(TypeError("programming bug")), + ) + + with pytest.raises(TypeError, match="programming bug"): + _place_skill_for_agent( + "my-skill", + skill_entry, + agent_config, + Path("/.napoln"), + Path.home(), + "global", + None, + ) diff --git a/tests/unit/test_install_logic.py b/tests/unit/test_install_logic.py new file mode 100644 index 0000000..091932b --- /dev/null +++ b/tests/unit/test_install_logic.py @@ -0,0 +1,85 @@ +"""Tests for napoln.commands.install — exception specificity.""" + +from __future__ import annotations + +import pytest + +from napoln.commands.install import _sync_manifest +from napoln.core import manifest as manifest_mod +from napoln.errors import NapolnError, PlacementError + + +class TestSyncManifestExceptions: + """Regression: broad except Exception swallows programming bugs.""" + + @pytest.fixture + def manifest_with_skill(self): + mf = manifest_mod.Manifest() + mf.skills["my-skill"] = manifest_mod.SkillEntry( + source="owner/repo", + version="1.0.0", + store_hash="abc123", + installed="2024-01-01T00:00:00Z", + updated="2024-01-01T00:00:00Z", + agents={ + "claude-code": manifest_mod.AgentPlacement( + path="~/.claude/skills/my-skill", + link_mode="clone", + scope="global", + ) + }, + ) + return mf + + def test_napoln_error_outputs_and_counts_error( + self, manifest_with_skill, monkeypatch, tmp_path + ): + errors = [] + + def capture_error(msg, **_kwargs): + errors.append(msg) + + monkeypatch.setattr("napoln.commands.install.output.error", capture_error) + monkeypatch.setattr( + "napoln.commands.install.store.ensure_stored", + lambda *_a, **_k: (_ for _ in ()).throw(NapolnError("store missing")), + ) + + synced, error_count = _sync_manifest(manifest_with_skill, "global", False) + assert error_count == 1 + + def test_placement_error_outputs_and_counts_error( + self, manifest_with_skill, monkeypatch, tmp_path + ): + errors = [] + + def capture_error(msg, **_kwargs): + errors.append(msg) + + monkeypatch.setattr("napoln.commands.install.output.error", capture_error) + monkeypatch.setattr( + "napoln.commands.install.store.ensure_stored", + lambda *_a, **_k: tmp_path / "store", + ) + monkeypatch.setattr( + "napoln.commands.install.linker.restore_placement", + lambda *_a, **_k: (_ for _ in ()).throw(PlacementError("placement failed")), + ) + + synced, error_count = _sync_manifest(manifest_with_skill, "global", False) + assert error_count == 1 + assert any("Failed to restore" in e for e in errors) + + def test_type_error_propagates(self, manifest_with_skill, monkeypatch, tmp_path): + """Programming bugs must not be swallowed.""" + monkeypatch.setattr( + "napoln.commands.install.store.ensure_stored", + lambda *_a, **_k: tmp_path / "store", + ) + monkeypatch.setattr( + "napoln.commands.install.linker.restore_placement", + lambda *_a, **_k: (_ for _ in ()).throw(TypeError("programming bug")), + ) + + with pytest.raises(TypeError, match="programming bug"): + _sync_manifest(manifest_with_skill, "global", False) diff --git a/tests/unit/test_manifest.py b/tests/unit/test_manifest.py index f250e9c..abb8e3b 100644 --- a/tests/unit/test_manifest.py +++ b/tests/unit/test_manifest.py @@ -218,6 +218,76 @@ def test_update_existing_skill(self): assert mf.skills["my-skill"].version == "2.0.0" assert mf.skills["my-skill"].store_hash == "def5678" + def test_read_manifest_wraps_toml_decode_error(self, tmp_path): + path = tmp_path / "manifest.toml" + path.write_text("[invalid toml", encoding="utf-8") + + from napoln.errors import ManifestError + + with pytest.raises(ManifestError, match="Could not read manifest"): + read_manifest(path) + + def test_read_manifest_wraps_os_error(self, tmp_path, monkeypatch): + path = tmp_path / "manifest.toml" + path.write_text("[napoln]\nschema = 1\n", encoding="utf-8") + + def raise_oserror(*_args, **_kwargs): + raise OSError("permission denied") + + monkeypatch.setattr("pathlib.Path.read_text", raise_oserror) + + from napoln.errors import ManifestError + + with pytest.raises(ManifestError, match="Could not read manifest"): + read_manifest(path) + + def test_read_manifest_propagates_type_error(self, tmp_path, monkeypatch): + """Programming bugs must not be swallowed.""" + path = tmp_path / "manifest.toml" + path.write_text("[napoln]\nschema = 1\n", encoding="utf-8") + + def raise_typeerror(*_args, **_kwargs): + raise TypeError("programming bug") + + monkeypatch.setattr("pathlib.Path.read_text", raise_typeerror) + with pytest.raises(TypeError, match="programming bug"): + read_manifest(path) + + def test_write_manifest_cleans_up_on_os_error(self, tmp_path, monkeypatch): + path = tmp_path / "manifest.toml" + mf = Manifest() + + real_write_text = type(path).write_text + + def failing_write_text(self, data, *args, **kwargs): + if self.name == ".manifest.toml.tmp": + raise OSError("disk full") + return real_write_text(self, data, *args, **kwargs) + + monkeypatch.setattr("pathlib.Path.write_text", failing_write_text) + + with pytest.raises(OSError, match="disk full"): + write_manifest(mf, path) + + assert not (tmp_path / ".manifest.toml.tmp").exists() + + def test_write_manifest_propagates_type_error(self, tmp_path, monkeypatch): + """Programming bugs must not be swallowed.""" + path = tmp_path / "manifest.toml" + mf = Manifest() + + real_write_text = type(path).write_text + + def failing_write_text(self, data, *args, **kwargs): + if self.name == ".manifest.toml.tmp": + raise TypeError("programming bug") + return real_write_text(self, data, *args, **kwargs) + + monkeypatch.setattr("pathlib.Path.write_text", failing_write_text) + + with pytest.raises(TypeError, match="programming bug"): + write_manifest(mf, path) + class TestRemoveSkillFromManifest: """Removing skills from the manifest.""" diff --git a/tests/unit/test_merger.py b/tests/unit/test_merger.py index 60f92d2..fef4a32 100644 --- a/tests/unit/test_merger.py +++ b/tests/unit/test_merger.py @@ -158,3 +158,35 @@ def test_detects_markers(self, tmp_path, content, expected): def test_nonexistent_file(self, tmp_path): assert has_conflict_markers(tmp_path / "nope.md") is False + + def test_returns_false_on_os_error(self, tmp_path, monkeypatch): + f = tmp_path / "test.md" + f.write_text("<<<<<<< local\nfoo\n=======\nbar\n>>>>>>> upstream\n") + + def raise_oserror(*_args, **_kwargs): + raise OSError("permission denied") + + monkeypatch.setattr("pathlib.Path.read_text", raise_oserror) + assert has_conflict_markers(f) is False + + def test_returns_false_on_unicode_decode_error(self, tmp_path, monkeypatch): + f = tmp_path / "test.md" + f.write_text("<<<<<<< local\nfoo\n=======\nbar\n>>>>>>> upstream\n") + + def raise_unicode(*_args, **_kwargs): + raise UnicodeDecodeError("utf-8", b"", 0, 1, "invalid") + + monkeypatch.setattr("pathlib.Path.read_text", raise_unicode) + assert has_conflict_markers(f) is False + + def test_propagates_type_error(self, tmp_path, monkeypatch): + """Programming bugs must not be swallowed.""" + f = tmp_path / "test.md" + f.write_text("<<<<<<< local\nfoo\n=======\nbar\n>>>>>>> upstream\n") + + def raise_typeerror(*_args, **_kwargs): + raise TypeError("programming bug") + + monkeypatch.setattr("pathlib.Path.read_text", raise_typeerror) + with pytest.raises(TypeError, match="programming bug"): + has_conflict_markers(f) diff --git a/tests/unit/test_resolver.py b/tests/unit/test_resolver.py index f2428ee..366a12a 100644 --- a/tests/unit/test_resolver.py +++ b/tests/unit/test_resolver.py @@ -7,6 +7,8 @@ from napoln.core import resolver from napoln.core.resolver import ( ParsedSource, + _extract_description, + _extract_version, _fetch_sentinel, _should_fetch, parse_source, @@ -292,3 +294,106 @@ def test_stale_sentinel_re_fetches(self, monkeypatch, git_parsed, populated_cach fetch_calls = [c for c in calls if c[:2] == ["git", "fetch"]] assert len(fetch_calls) == 2 + + +class TestExtractVersion: + """Regression: broad except Exception swallows programming bugs.""" + + def test_returns_default_on_yaml_error(self, tmp_path): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\n[invalid yaml\n---\n# Hello") + + import yaml + + # Ensure yaml.safe_load actually raises YAMLError for this content + with pytest.raises(yaml.YAMLError): + yaml.safe_load("[invalid yaml") + + assert _extract_version(skill_dir) == "0.0.0" + + def test_returns_default_on_os_error(self, tmp_path, monkeypatch): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\nversion: 1.0.0\n---\n") + + def raise_oserror(*_args, **_kwargs): + raise OSError("permission denied") + + monkeypatch.setattr("pathlib.Path.read_text", raise_oserror) + assert _extract_version(skill_dir) == "0.0.0" + + def test_returns_default_on_unicode_decode_error(self, tmp_path, monkeypatch): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\nversion: 1.0.0\n---\n") + + def raise_unicode(*_args, **_kwargs): + raise UnicodeDecodeError("utf-8", b"", 0, 1, "invalid") + + monkeypatch.setattr("pathlib.Path.read_text", raise_unicode) + assert _extract_version(skill_dir) == "0.0.0" + + def test_propagates_type_error(self, tmp_path, monkeypatch): + """Programming bugs must not be swallowed.""" + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\nversion: 1.0.0\n---\n") + + def raise_typeerror(*_args, **_kwargs): + raise TypeError("programming bug") + + monkeypatch.setattr("pathlib.Path.read_text", raise_typeerror) + with pytest.raises(TypeError, match="programming bug"): + _extract_version(skill_dir) + + +class TestExtractDescription: + """Regression: broad except Exception swallows programming bugs.""" + + def test_returns_empty_on_yaml_error(self, tmp_path): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\n[invalid yaml\n---\n# Hello") + + import yaml + + with pytest.raises(yaml.YAMLError): + yaml.safe_load("[invalid yaml") + + assert _extract_description(skill_dir) == "" + + def test_returns_empty_on_os_error(self, tmp_path, monkeypatch): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\ndescription: hello\n---\n") + + def raise_oserror(*_args, **_kwargs): + raise OSError("permission denied") + + monkeypatch.setattr("pathlib.Path.read_text", raise_oserror) + assert _extract_description(skill_dir) == "" + + def test_returns_empty_on_unicode_decode_error(self, tmp_path, monkeypatch): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\ndescription: hello\n---\n") + + def raise_unicode(*_args, **_kwargs): + raise UnicodeDecodeError("utf-8", b"", 0, 1, "invalid") + + monkeypatch.setattr("pathlib.Path.read_text", raise_unicode) + assert _extract_description(skill_dir) == "" + + def test_propagates_type_error(self, tmp_path, monkeypatch): + """Programming bugs must not be swallowed.""" + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + skill_dir.joinpath("SKILL.md").write_text("---\ndescription: hello\n---\n") + + def raise_typeerror(*_args, **_kwargs): + raise TypeError("programming bug") + + monkeypatch.setattr("pathlib.Path.read_text", raise_typeerror) + with pytest.raises(TypeError, match="programming bug"): + _extract_description(skill_dir) diff --git a/tests/unit/test_store.py b/tests/unit/test_store.py index 883dd54..bc79a22 100644 --- a/tests/unit/test_store.py +++ b/tests/unit/test_store.py @@ -110,3 +110,36 @@ def test_corrupted_entry(self, skill_builder, store_home): (store_path / "SKILL.md").write_text("CORRUPTED") assert verify_store_entry(store_path) is False + + def test_cleans_up_temp_on_os_error(self, skill_builder, store_home, monkeypatch): + """OSError during copytree should clean up the temp directory.""" + skill_dir = skill_builder("my-skill") + import shutil + + def failing_copytree(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr(shutil, "copytree", failing_copytree) + + with pytest.raises(OSError, match="disk full"): + store_skill(skill_dir, "my-skill", "1.0.0", store_home) + + # No stray temp directory should remain + skill_store = store_home / "store" / "my-skill" + if skill_store.exists(): + temps = [p for p in skill_store.iterdir() if p.name.startswith(".")] + assert temps == [], f"unexpected temp dirs: {temps}" + + def test_propagates_type_error(self, skill_builder, store_home, monkeypatch): + """Programming bugs must not be swallowed.""" + skill_dir = skill_builder("my-skill") + + import shutil + + def failing_copytree(*_args, **_kwargs): + raise TypeError("programming bug") + + monkeypatch.setattr(shutil, "copytree", failing_copytree) + + with pytest.raises(TypeError, match="programming bug"): + store_skill(skill_dir, "my-skill", "1.0.0", store_home)