Skip to content

Commit 2bf655e

Browse files
mbachorikiamaeroplaneclaude
authored
feat(presets): add enable/disable toggle and update semantics (#1891)
* feat(presets): add enable/disable toggle and update semantics Add preset enable/disable CLI commands and update semantics to match the extension system capabilities. Changes: - Add `preset enable` and `preset disable` CLI commands - Add `restore()` method to PresetRegistry for rollback scenarios - Update `get()` and `list()` to return deep copies (prevents mutation) - Update `list_by_priority()` to filter disabled presets by default - Add input validation to `restore()` for defensive programming - Add 16 new tests covering all functionality and edge cases Closes #1851 Closes #1852 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review - deep copy and error message accuracy - Fix error message in restore() to match actual validation ("dict" not "non-empty dict") - Use copy.deepcopy() in restore() to prevent caller mutation - Apply same fixes to ExtensionRegistry for parity - Add /defensive-check command for pre-PR validation - Add tests for restore() validation and deep copy behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * revert: remove defensive-check command from PR * fix: address PR review - clarify messaging and add parity - Add note to enable/disable output clarifying commands/skills remain active - Add include_disabled parameter to ExtensionRegistry.list_by_priority for parity - Add tests for extension disabled filtering Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * 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> * fix: handle corrupted registry in list() methods - Add defensive handling to list() when presets/extensions is not a dict - Return empty dict instead of crashing on corrupted registry - Apply same fix to both PresetRegistry and ExtensionRegistry for parity - Add tests for corrupted registry handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: validate top-level registry structure in get() and restore() - get() now validates self.data["presets/extensions"] is a dict before accessing - restore() ensures presets/extensions dict exists before writing - Prevents crashes when registry JSON is parseable but has corrupted structure - Applied same fixes to both PresetRegistry and ExtensionRegistry for parity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: validate root-level JSON structure in _load() and is_installed() - _load() now validates json.load() result is a dict before returning - is_installed() validates presets/extensions is a dict before checking membership - Prevents crashes when registry file is valid JSON but wrong type (e.g., array) - Applied same fixes to both registries for parity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: normalize presets/extensions field in _load() - _load() now normalizes the presets/extensions field to {} if not a dict - Makes corrupted registries recoverable for add/update/remove operations - Applied same fix to both registries for parity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use raw registry keys to track corrupted extensions - Use registry.list().keys() instead of list_by_priority() for tracking - Corrupted entries are now treated as tracked, not picked up as unregistered - Tighten test assertion for disabled preset resolution - Update test to match new expected behavior for corrupted entries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle None metadata in ExtensionManager.remove() - Add defensive check for corrupted metadata in remove() - Match existing pattern in PresetManager.remove() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add keys() method and filter corrupted entries in list() - Add lightweight keys() method that returns IDs without deep copy - Update list() to filter out non-dict entries (match type contract) - Use keys() instead of list().keys() for performance - Fix comment to reflect actual behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address defensive-check findings - deep copy, corruption guards, parity - Extension enable/disable: use delta pattern matching presets - add(): use copy.deepcopy(metadata) in both registries - remove(): guard outer field for corruption in both registries - update(): guard outer field for corruption in both registries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: deep copy updates in update() to prevent caller mutation Both PresetRegistry.update() and ExtensionRegistry.update() now deep copy the input updates/metadata dict to prevent callers from mutating nested objects after the call. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: iamaeroplane <michal.bachorik@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent f679468 commit 2bf655e

File tree

5 files changed

+783
-54
lines changed

5 files changed

+783
-54
lines changed

src/specify_cli/__init__.py

Lines changed: 85 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2419,6 +2419,89 @@ def preset_set_priority(
24192419
console.print("\n[dim]Lower priority = higher precedence in template resolution[/dim]")
24202420

24212421

2422+
@preset_app.command("enable")
2423+
def preset_enable(
2424+
pack_id: str = typer.Argument(help="Preset ID to enable"),
2425+
):
2426+
"""Enable a disabled preset."""
2427+
from .presets import PresetManager
2428+
2429+
project_root = Path.cwd()
2430+
2431+
# Check if we're in a spec-kit project
2432+
specify_dir = project_root / ".specify"
2433+
if not specify_dir.exists():
2434+
console.print("[red]Error:[/red] Not a spec-kit project (no .specify/ directory)")
2435+
console.print("Run this command from a spec-kit project root")
2436+
raise typer.Exit(1)
2437+
2438+
manager = PresetManager(project_root)
2439+
2440+
# Check if preset is installed
2441+
if not manager.registry.is_installed(pack_id):
2442+
console.print(f"[red]Error:[/red] Preset '{pack_id}' is not installed")
2443+
raise typer.Exit(1)
2444+
2445+
# Get current metadata
2446+
metadata = manager.registry.get(pack_id)
2447+
if metadata is None or not isinstance(metadata, dict):
2448+
console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)")
2449+
raise typer.Exit(1)
2450+
2451+
if metadata.get("enabled", True):
2452+
console.print(f"[yellow]Preset '{pack_id}' is already enabled[/yellow]")
2453+
raise typer.Exit(0)
2454+
2455+
# Enable the preset
2456+
manager.registry.update(pack_id, {"enabled": True})
2457+
2458+
console.print(f"[green]✓[/green] Preset '{pack_id}' enabled")
2459+
console.print("\nTemplates from this preset will now be included in resolution.")
2460+
console.print("[dim]Note: Previously registered commands/skills remain active.[/dim]")
2461+
2462+
2463+
@preset_app.command("disable")
2464+
def preset_disable(
2465+
pack_id: str = typer.Argument(help="Preset ID to disable"),
2466+
):
2467+
"""Disable a preset without removing it."""
2468+
from .presets import PresetManager
2469+
2470+
project_root = Path.cwd()
2471+
2472+
# Check if we're in a spec-kit project
2473+
specify_dir = project_root / ".specify"
2474+
if not specify_dir.exists():
2475+
console.print("[red]Error:[/red] Not a spec-kit project (no .specify/ directory)")
2476+
console.print("Run this command from a spec-kit project root")
2477+
raise typer.Exit(1)
2478+
2479+
manager = PresetManager(project_root)
2480+
2481+
# Check if preset is installed
2482+
if not manager.registry.is_installed(pack_id):
2483+
console.print(f"[red]Error:[/red] Preset '{pack_id}' is not installed")
2484+
raise typer.Exit(1)
2485+
2486+
# Get current metadata
2487+
metadata = manager.registry.get(pack_id)
2488+
if metadata is None or not isinstance(metadata, dict):
2489+
console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)")
2490+
raise typer.Exit(1)
2491+
2492+
if not metadata.get("enabled", True):
2493+
console.print(f"[yellow]Preset '{pack_id}' is already disabled[/yellow]")
2494+
raise typer.Exit(0)
2495+
2496+
# Disable the preset
2497+
manager.registry.update(pack_id, {"enabled": False})
2498+
2499+
console.print(f"[green]✓[/green] Preset '{pack_id}' disabled")
2500+
console.print("\nTemplates from this preset will be skipped during resolution.")
2501+
console.print("[dim]Note: Previously registered commands/skills remain active until preset removal.[/dim]")
2502+
console.print(f"To re-enable: specify preset enable {pack_id}")
2503+
2504+
24222505
# ===== Preset Catalog Commands =====
24232506

24242507

@@ -3855,8 +3938,7 @@ def extension_enable(
38553938
console.print(f"[yellow]Extension '{display_name}' is already enabled[/yellow]")
38563939
raise typer.Exit(0)
38573940

3858-
metadata["enabled"] = True
3859-
manager.registry.update(extension_id, metadata)
3941+
manager.registry.update(extension_id, {"enabled": True})
38603942

38613943
# Enable hooks in extensions.yml
38623944
config = hook_executor.get_project_config()
@@ -3903,8 +3985,7 @@ def extension_disable(
39033985
console.print(f"[yellow]Extension '{display_name}' is already disabled[/yellow]")
39043986
raise typer.Exit(0)
39053987

3906-
metadata["enabled"] = False
3907-
manager.registry.update(extension_id, metadata)
3988+
manager.registry.update(extension_id, {"enabled": False})
39083989

39093990
# Disable hooks in extensions.yml
39103991
config = hook_executor.get_project_config()

src/specify_cli/extensions.py

Lines changed: 81 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,17 @@ def _load(self) -> dict:
222222

223223
try:
224224
with open(self.registry_path, 'r') as f:
225-
return json.load(f)
225+
data = json.load(f)
226+
# Validate loaded data is a dict (handles corrupted registry files)
227+
if not isinstance(data, dict):
228+
return {
229+
"schema_version": self.SCHEMA_VERSION,
230+
"extensions": {}
231+
}
232+
# Normalize extensions field (handles corrupted extensions value)
233+
if not isinstance(data.get("extensions"), dict):
234+
data["extensions"] = {}
235+
return data
226236
except (json.JSONDecodeError, FileNotFoundError):
227237
# Corrupted or missing registry, start fresh
228238
return {
@@ -244,7 +254,7 @@ def add(self, extension_id: str, metadata: dict):
244254
metadata: Extension metadata (version, source, etc.)
245255
"""
246256
self.data["extensions"][extension_id] = {
247-
**metadata,
257+
**copy.deepcopy(metadata),
248258
"installed_at": datetime.now(timezone.utc).isoformat()
249259
}
250260
self._save()
@@ -267,23 +277,24 @@ def update(self, extension_id: str, metadata: dict):
267277
Raises:
268278
KeyError: If extension is not installed
269279
"""
270-
if extension_id not in self.data["extensions"]:
280+
extensions = self.data.get("extensions")
281+
if not isinstance(extensions, dict) or extension_id not in extensions:
271282
raise KeyError(f"Extension '{extension_id}' is not installed")
272283
# Merge new metadata with existing, preserving original installed_at
273-
existing = self.data["extensions"][extension_id]
284+
existing = extensions[extension_id]
274285
# Handle corrupted registry entries (e.g., string/list instead of dict)
275286
if not isinstance(existing, dict):
276287
existing = {}
277-
# Merge: existing fields preserved, new fields override
278-
merged = {**existing, **metadata}
288+
# Merge: existing fields preserved, new fields override (deep copy to prevent caller mutation)
289+
merged = {**existing, **copy.deepcopy(metadata)}
279290
# Always preserve original installed_at based on key existence, not truthiness,
280291
# to handle cases where the field exists but may be falsy (legacy/corruption)
281292
if "installed_at" in existing:
282293
merged["installed_at"] = existing["installed_at"]
283294
else:
284295
# If not present in existing, explicitly remove from merged if caller provided it
285296
merged.pop("installed_at", None)
286-
self.data["extensions"][extension_id] = merged
297+
extensions[extension_id] = merged
287298
self._save()
288299

289300
def restore(self, extension_id: str, metadata: dict):
@@ -296,8 +307,16 @@ def restore(self, extension_id: str, metadata: dict):
296307
Args:
297308
extension_id: Extension ID
298309
metadata: Complete extension metadata including installed_at
310+
311+
Raises:
312+
ValueError: If metadata is None or not a dict
299313
"""
300-
self.data["extensions"][extension_id] = dict(metadata)
314+
if metadata is None or not isinstance(metadata, dict):
315+
raise ValueError(f"Cannot restore '{extension_id}': metadata must be a dict")
316+
# Ensure extensions dict exists (handle corrupted registry)
317+
if not isinstance(self.data.get("extensions"), dict):
318+
self.data["extensions"] = {}
319+
self.data["extensions"][extension_id] = copy.deepcopy(metadata)
301320
self._save()
302321

303322
def remove(self, extension_id: str):
@@ -306,8 +325,11 @@ def remove(self, extension_id: str):
306325
Args:
307326
extension_id: Extension ID
308327
"""
309-
if extension_id in self.data["extensions"]:
310-
del self.data["extensions"][extension_id]
328+
extensions = self.data.get("extensions")
329+
if not isinstance(extensions, dict):
330+
return
331+
if extension_id in extensions:
332+
del extensions[extension_id]
311333
self._save()
312334

313335
def get(self, extension_id: str) -> Optional[dict]:
@@ -320,21 +342,49 @@ def get(self, extension_id: str) -> Optional[dict]:
320342
extension_id: Extension ID
321343
322344
Returns:
323-
Deep copy of extension metadata, or None if not found
345+
Deep copy of extension metadata, or None if not found or corrupted
324346
"""
325-
entry = self.data["extensions"].get(extension_id)
326-
return copy.deepcopy(entry) if entry is not None else None
347+
extensions = self.data.get("extensions")
348+
if not isinstance(extensions, dict):
349+
return None
350+
entry = extensions.get(extension_id)
351+
# Return None for missing or corrupted (non-dict) entries
352+
if entry is None or not isinstance(entry, dict):
353+
return None
354+
return copy.deepcopy(entry)
327355

328356
def list(self) -> Dict[str, dict]:
329-
"""Get all installed extensions.
357+
"""Get all installed extensions with valid metadata.
358+
359+
Returns a deep copy of extensions with dict metadata only.
360+
Corrupted entries (non-dict values) are filtered out.
361+
362+
Returns:
363+
Dictionary of extension_id -> metadata (deep copies), empty dict if corrupted
364+
"""
365+
extensions = self.data.get("extensions", {}) or {}
366+
if not isinstance(extensions, dict):
367+
return {}
368+
# Filter to only valid dict entries to match type contract
369+
return {
370+
ext_id: copy.deepcopy(meta)
371+
for ext_id, meta in extensions.items()
372+
if isinstance(meta, dict)
373+
}
374+
375+
def keys(self) -> set:
376+
"""Get all extension IDs including corrupted entries.
330377
331-
Returns a deep copy of the extensions mapping to prevent callers
332-
from accidentally mutating nested internal registry state.
378+
Lightweight method that returns IDs without deep-copying metadata.
379+
Use this when you only need to check which extensions are tracked.
333380
334381
Returns:
335-
Dictionary of extension_id -> metadata (deep copies)
382+
Set of extension IDs (includes corrupted entries)
336383
"""
337-
return copy.deepcopy(self.data["extensions"])
384+
extensions = self.data.get("extensions", {}) or {}
385+
if not isinstance(extensions, dict):
386+
return set()
387+
return set(extensions.keys())
338388

339389
def is_installed(self, extension_id: str) -> bool:
340390
"""Check if extension is installed.
@@ -343,17 +393,23 @@ def is_installed(self, extension_id: str) -> bool:
343393
extension_id: Extension ID
344394
345395
Returns:
346-
True if extension is installed
396+
True if extension is installed, False if not or registry corrupted
347397
"""
348-
return extension_id in self.data["extensions"]
398+
extensions = self.data.get("extensions")
399+
if not isinstance(extensions, dict):
400+
return False
401+
return extension_id in extensions
349402

350-
def list_by_priority(self) -> List[tuple]:
403+
def list_by_priority(self, include_disabled: bool = False) -> List[tuple]:
351404
"""Get all installed extensions sorted by priority.
352405
353406
Lower priority number = higher precedence (checked first).
354407
Extensions with equal priority are sorted alphabetically by ID
355408
for deterministic ordering.
356409
410+
Args:
411+
include_disabled: If True, include disabled extensions. Default False.
412+
357413
Returns:
358414
List of (extension_id, metadata_copy) tuples sorted by priority.
359415
Metadata is deep-copied to prevent accidental mutation.
@@ -365,6 +421,9 @@ def list_by_priority(self) -> List[tuple]:
365421
for ext_id, meta in extensions.items():
366422
if not isinstance(meta, dict):
367423
continue
424+
# Skip disabled extensions unless explicitly requested
425+
if not include_disabled and not meta.get("enabled", True):
426+
continue
368427
metadata_copy = copy.deepcopy(meta)
369428
metadata_copy["priority"] = normalize_priority(metadata_copy.get("priority", 10))
370429
sortable_extensions.append((ext_id, metadata_copy))
@@ -633,7 +692,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool:
633692

634693
# Get registered commands before removal
635694
metadata = self.registry.get(extension_id)
636-
registered_commands = metadata.get("registered_commands", {})
695+
registered_commands = metadata.get("registered_commands", {}) if metadata else {}
637696

638697
extension_dir = self.extensions_dir / extension_id
639698

0 commit comments

Comments
 (0)