Skip to content

Commit 6978238

Browse files
iamaeroplaneclaude
andcommitted
fix: address PR review - disabled extension resolution and corrupted entries
- Fix _get_all_extensions_by_priority to use include_disabled=True for tracking registered IDs, preventing disabled extensions from being picked up as unregistered directories - Add corrupted entry handling to get() - returns None for non-dict entries - Add integration tests for disabled extension template resolution - Add tests for get() corrupted entry handling in both registries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 162573b commit 6978238

4 files changed

Lines changed: 95 additions & 7 deletions

File tree

src/specify_cli/extensions.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,10 +325,13 @@ def get(self, extension_id: str) -> Optional[dict]:
325325
extension_id: Extension ID
326326
327327
Returns:
328-
Deep copy of extension metadata, or None if not found
328+
Deep copy of extension metadata, or None if not found or corrupted
329329
"""
330330
entry = self.data["extensions"].get(extension_id)
331-
return copy.deepcopy(entry) if entry is not None else None
331+
# Return None for missing or corrupted (non-dict) entries
332+
if entry is None or not isinstance(entry, dict):
333+
return None
334+
return copy.deepcopy(entry)
332335

333336
def list(self) -> Dict[str, dict]:
334337
"""Get all installed extensions.

src/specify_cli/presets.py

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -335,10 +335,13 @@ def get(self, pack_id: str) -> Optional[dict]:
335335
pack_id: Preset ID
336336
337337
Returns:
338-
Deep copy of preset metadata, or None if not found
338+
Deep copy of preset metadata, or None if not found or corrupted
339339
"""
340340
entry = self.data["presets"].get(pack_id)
341-
return copy.deepcopy(entry) if entry is not None else None
341+
# Return None for missing or corrupted (non-dict) entries
342+
if entry is None or not isinstance(entry, dict):
343+
return None
344+
return copy.deepcopy(entry)
342345

343346
def list(self) -> Dict[str, dict]:
344347
"""Get all installed presets.
@@ -1498,12 +1501,17 @@ def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]:
14981501
return []
14991502

15001503
registry = ExtensionRegistry(self.extensions_dir)
1501-
registered_extensions = registry.list_by_priority()
1502-
registered_extension_ids = {ext_id for ext_id, _ in registered_extensions}
1504+
# Get ALL registered extensions (including disabled) to know which dirs are tracked
1505+
all_registered = registry.list_by_priority(include_disabled=True)
1506+
registered_extension_ids = {ext_id for ext_id, _ in all_registered}
15031507

15041508
all_extensions: list[tuple[int, str, dict | None]] = []
15051509

1506-
for ext_id, metadata in registered_extensions:
1510+
# Only include enabled extensions in the result
1511+
for ext_id, metadata in all_registered:
1512+
# Skip disabled extensions
1513+
if not metadata.get("enabled", True):
1514+
continue
15071515
priority = normalize_priority(metadata.get("priority") if metadata else None)
15081516
all_extensions.append((priority, ext_id, metadata))
15091517

tests/test_extensions.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,26 @@ def test_get_returns_deep_copy(self, temp_dir):
481481
internal = registry.data["extensions"]["test-ext"]
482482
assert internal["registered_commands"] == {"claude": ["cmd1"]}
483483

484+
def test_get_returns_none_for_corrupted_entry(self, temp_dir):
485+
"""Test that get() returns None for corrupted (non-dict) entries."""
486+
extensions_dir = temp_dir / "extensions"
487+
extensions_dir.mkdir()
488+
489+
registry = ExtensionRegistry(extensions_dir)
490+
491+
# Directly corrupt the registry with non-dict entries
492+
registry.data["extensions"]["corrupted-string"] = "not a dict"
493+
registry.data["extensions"]["corrupted-list"] = ["not", "a", "dict"]
494+
registry.data["extensions"]["corrupted-int"] = 42
495+
registry._save()
496+
497+
# All corrupted entries should return None
498+
assert registry.get("corrupted-string") is None
499+
assert registry.get("corrupted-list") is None
500+
assert registry.get("corrupted-int") is None
501+
# Non-existent should also return None
502+
assert registry.get("nonexistent") is None
503+
484504
def test_list_returns_deep_copy(self, temp_dir):
485505
"""Test that list() returns deep copies for nested structures."""
486506
extensions_dir = temp_dir / "extensions"

tests/test_presets.py

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,25 @@ def test_get_returns_deep_copy(self, temp_dir):
450450
assert fresh["version"] == "1.0.0"
451451
assert fresh["nested"]["key"] == "original"
452452

453+
def test_get_returns_none_for_corrupted_entry(self, temp_dir):
454+
"""Test that get() returns None for corrupted (non-dict) entries."""
455+
packs_dir = temp_dir / "packs"
456+
packs_dir.mkdir()
457+
registry = PresetRegistry(packs_dir)
458+
459+
# Directly corrupt the registry with non-dict entries
460+
registry.data["presets"]["corrupted-string"] = "not a dict"
461+
registry.data["presets"]["corrupted-list"] = ["not", "a", "dict"]
462+
registry.data["presets"]["corrupted-int"] = 42
463+
registry._save()
464+
465+
# All corrupted entries should return None
466+
assert registry.get("corrupted-string") is None
467+
assert registry.get("corrupted-list") is None
468+
assert registry.get("corrupted-int") is None
469+
# Non-existent should also return None
470+
assert registry.get("nonexistent") is None
471+
453472
def test_list_returns_deep_copy(self, temp_dir):
454473
"""Test that list() returns deep copies to prevent mutation."""
455474
packs_dir = temp_dir / "packs"
@@ -840,6 +859,44 @@ def test_resolve_extension_provided_templates(self, project_dir):
840859
assert result is not None
841860
assert "Extension Custom Template" in result.read_text()
842861

862+
def test_resolve_disabled_extension_templates_skipped(self, project_dir):
863+
"""Test that disabled extension templates are not resolved."""
864+
# Create extension with templates
865+
ext_dir = project_dir / ".specify" / "extensions" / "disabled-ext"
866+
ext_templates_dir = ext_dir / "templates"
867+
ext_templates_dir.mkdir(parents=True)
868+
ext_template = ext_templates_dir / "disabled-template.md"
869+
ext_template.write_text("# Disabled Extension Template\n")
870+
871+
# Register extension as disabled
872+
extensions_dir = project_dir / ".specify" / "extensions"
873+
ext_registry = ExtensionRegistry(extensions_dir)
874+
ext_registry.add("disabled-ext", {"version": "1.0.0", "priority": 1, "enabled": False})
875+
876+
# Template should NOT be resolved because extension is disabled
877+
resolver = PresetResolver(project_dir)
878+
result = resolver.resolve("disabled-template")
879+
assert result is None, "Disabled extension template should not be resolved"
880+
881+
def test_resolve_disabled_extension_not_picked_up_as_unregistered(self, project_dir):
882+
"""Test that disabled extensions are not picked up via unregistered dir scan."""
883+
# Create extension directory with templates
884+
ext_dir = project_dir / ".specify" / "extensions" / "test-disabled-ext"
885+
ext_templates_dir = ext_dir / "templates"
886+
ext_templates_dir.mkdir(parents=True)
887+
ext_template = ext_templates_dir / "unique-disabled-template.md"
888+
ext_template.write_text("# Should Not Resolve\n")
889+
890+
# Register the extension but disable it
891+
extensions_dir = project_dir / ".specify" / "extensions"
892+
ext_registry = ExtensionRegistry(extensions_dir)
893+
ext_registry.add("test-disabled-ext", {"version": "1.0.0", "enabled": False})
894+
895+
# Verify the template is NOT resolved (even though the directory exists)
896+
resolver = PresetResolver(project_dir)
897+
result = resolver.resolve("unique-disabled-template")
898+
assert result is None, "Disabled extension should not be picked up as unregistered"
899+
843900
def test_resolve_pack_over_extension(self, project_dir, pack_dir, temp_dir, valid_pack_data):
844901
"""Test that pack templates take priority over extension templates."""
845902
# Create extension with templates

0 commit comments

Comments
 (0)