mirror of
https://github.com/github/spec-kit.git
synced 2026-03-22 05:13:08 +00:00
feat(extensions): Quality of life improvements for RFC-aligned catalog integration (#1776)
* feat(extensions): implement automatic updates with atomic backup/restore - Implement automatic extension updates with download from catalog - Add comprehensive backup/restore mechanism for failed updates: - Backup registry entry before update - Backup extension directory - Backup command files for all AI agents - Backup hooks from extensions.yml - Add extension ID verification after install - Add KeyboardInterrupt handling to allow clean cancellation - Fix enable/disable to preserve installed_at timestamp by using direct registry manipulation instead of registry.add() - Add rollback on any update failure with command file, hook, and registry restoration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): comprehensive name resolution and error handling improvements - Add shared _resolve_installed_extension helper for ID/display name resolution with proper ambiguous name handling (shows table of matches) - Add _resolve_catalog_extension helper for catalog lookups by ID or display name - Update enable/disable/update/remove commands to use name resolution helpers - Fix extension_info to handle catalog errors gracefully: - Fallback to local installed info when catalog unavailable - Distinguish "catalog unavailable" from "not found in catalog" - Support display name lookup for both installed and catalog extensions - Use resolved display names in all status messages for consistency - Extract _print_extension_info helper for DRY catalog info printing Addresses reviewer feedback: - Ambiguous name handling in enable/disable/update - Catalog error fallback for installed extensions - UX message clarity (catalog unavailable vs not found) - Resolved ID in status messages Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): properly detect ambiguous names in extension_info The extension_info command was breaking on the first name match without checking for ambiguity. This fix separates ID matching from name matching and checks for ambiguity before selecting a match, consistent with the _resolve_installed_extension() helper used by other commands. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor(extensions): add public update() method to ExtensionRegistry Add a proper public API for updating registry metadata while preserving installed_at timestamp, instead of directly mutating internal registry data and calling private _save() method. Changes: - Add ExtensionRegistry.update() method that preserves installed_at - Update enable/disable commands to use registry.update() - Update rollback logic to use registry.update() This decouples the CLI from registry internals and maintains proper encapsulation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): safely access optional author field in extension_info ExtensionManifest doesn't expose an author property - the author field is optional in extension.yml and stored in data["extension"]["author"]. Use safe dict access to avoid AttributeError. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): address multiple reviewer comments - ExtensionRegistry.update() now preserves original installed_at timestamp - Add ExtensionRegistry.restore() for rollback (entry was removed) - Clean up wrongly installed extension on ID mismatch before rollback - Remove unused catalog_error parameter from _print_extension_info() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): check _install_allowed for updates, preserve backup on failed rollback - Skip automatic updates for extensions from catalogs with install_allowed=false - Only delete backup directory on successful rollback, preserve it on failure for manual recovery Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): address reviewer feedback on update/rollback logic - Hook rollback: handle empty backup_hooks by checking `is not None` instead of truthiness (falsy empty dict would skip hook cleanup) - extension_info: use resolved_installed_id for catalog lookup when extension was found by display name (prevents wrong catalog match) - Rollback: always remove extension dir first, then restore if backup exists (handles case when no original dir existed before update) - Validate extension ID from ZIP before installing, not after (avoids side effects of installing wrong extension before rollback) - Preserve enabled state during updates: re-apply disabled state and hook enabled flags after successful update - Optimize _resolve_catalog_extension: pass query to catalog.search() instead of fetching all extensions - update() now merges metadata with existing entry instead of replacing (preserves fields like registered_commands when only updating enabled) - Add tests for ExtensionRegistry.update() and restore() methods: - test_update_preserves_installed_at - test_update_merges_with_existing - test_update_raises_for_missing_extension - test_restore_overwrites_completely - test_restore_can_recreate_removed_entry Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * docs(extensions): update RFC to reflect implemented status - Change status from "Draft" to "Implemented" - Update all Implementation Phases to show completed items - Add new features implemented beyond original RFC: - Display name resolution for all commands - Ambiguous name handling with tables - Atomic update with rollback - Pre-install ID validation - Enabled state preservation - Registry update/restore methods - Catalog error fallback - _install_allowed flag - Cache invalidation - Convert Open Questions to Resolved Questions with decisions - Add remaining Open Questions (sandboxing, signatures) as future work - Fix table of contents links Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): address third round of PR review comments - Refactor extension_info to use _resolve_installed_extension() helper with new allow_not_found parameter instead of duplicating resolution logic - Fix rollback hook restoration to not create empty hooks: {} in config when original config had no hooks section - Fix ZIP pre-validation to handle nested extension.yml files (GitHub auto-generated ZIPs have structure like repo-name-branch/extension.yml) - Replace unused installed_manifest variable with _ placeholder - Add display name to update status messages for better UX Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(extensions): address fourth round of PR review comments Rollback fixes: - Preserve installed_at timestamp after successful update (was reset by install_from_zip calling registry.add) - Fix rollback to only delete extension_dir if backup exists (avoids destroying valid installation when failure happens before modification) - Fix rollback to remove NEW command files created by failed install (files that weren't in original backup are now cleaned up) - Fix rollback to delete hooks key entirely when backup_hooks is None (original config had no hooks key, so restore should remove it) Cross-command consistency fix: - Add display name resolution to `extension add` command using _resolve_catalog_extension() helper (was only in `extension info`) - Use resolved extension ID for download_extension() call, not original argument which may be a display name Security fix (fail-closed): - Malformed catalog config (empty/missing URLs) now raises ValidationError instead of silently falling back to built-in catalogs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(lint): address ruff linting errors and registry.update() semantics - Remove unused import ExtensionError in extension_info - Remove extraneous f-prefix from strings without placeholders - Use registry.restore() instead of registry.update() for installed_at preservation (update() always preserves existing installed_at, ignoring our override) 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>
This commit is contained in:
Michal Bachorik
@@ -12,6 +12,7 @@ import os
|
||||
import tempfile
|
||||
import zipfile
|
||||
import shutil
|
||||
import copy
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import Optional, Dict, List, Any, Callable, Set
|
||||
@@ -228,6 +229,54 @@ class ExtensionRegistry:
|
||||
}
|
||||
self._save()
|
||||
|
||||
def update(self, extension_id: str, metadata: dict):
|
||||
"""Update extension metadata in registry, merging with existing entry.
|
||||
|
||||
Merges the provided metadata with the existing entry, preserving any
|
||||
fields not specified in the new metadata. The installed_at timestamp
|
||||
is always preserved from the original entry.
|
||||
|
||||
Use this method instead of add() when updating existing extension
|
||||
metadata (e.g., enabling/disabling) to preserve the original
|
||||
installation timestamp and other existing fields.
|
||||
|
||||
Args:
|
||||
extension_id: Extension ID
|
||||
metadata: Extension metadata fields to update (merged with existing)
|
||||
|
||||
Raises:
|
||||
KeyError: If extension is not installed
|
||||
"""
|
||||
if extension_id not in self.data["extensions"]:
|
||||
raise KeyError(f"Extension '{extension_id}' is not installed")
|
||||
# Merge new metadata with existing, preserving original installed_at
|
||||
existing = self.data["extensions"][extension_id]
|
||||
# Merge: existing fields preserved, new fields override
|
||||
merged = {**existing, **metadata}
|
||||
# Always preserve original installed_at based on key existence, not truthiness,
|
||||
# to handle cases where the field exists but may be falsy (legacy/corruption)
|
||||
if "installed_at" in existing:
|
||||
merged["installed_at"] = existing["installed_at"]
|
||||
else:
|
||||
# If not present in existing, explicitly remove from merged if caller provided it
|
||||
merged.pop("installed_at", None)
|
||||
self.data["extensions"][extension_id] = merged
|
||||
self._save()
|
||||
|
||||
def restore(self, extension_id: str, metadata: dict):
|
||||
"""Restore extension metadata to registry without modifying timestamps.
|
||||
|
||||
Use this method for rollback scenarios where you have a complete backup
|
||||
of the registry entry (including installed_at) and want to restore it
|
||||
exactly as it was.
|
||||
|
||||
Args:
|
||||
extension_id: Extension ID
|
||||
metadata: Complete extension metadata including installed_at
|
||||
"""
|
||||
self.data["extensions"][extension_id] = dict(metadata)
|
||||
self._save()
|
||||
|
||||
def remove(self, extension_id: str):
|
||||
"""Remove extension from registry.
|
||||
|
||||
@@ -241,21 +290,28 @@ class ExtensionRegistry:
|
||||
def get(self, extension_id: str) -> Optional[dict]:
|
||||
"""Get extension metadata from registry.
|
||||
|
||||
Returns a deep copy to prevent callers from accidentally mutating
|
||||
nested internal registry state without going through the write path.
|
||||
|
||||
Args:
|
||||
extension_id: Extension ID
|
||||
|
||||
Returns:
|
||||
Extension metadata or None if not found
|
||||
Deep copy of extension metadata, or None if not found
|
||||
"""
|
||||
return self.data["extensions"].get(extension_id)
|
||||
entry = self.data["extensions"].get(extension_id)
|
||||
return copy.deepcopy(entry) if entry is not None else None
|
||||
|
||||
def list(self) -> Dict[str, dict]:
|
||||
"""Get all installed extensions.
|
||||
|
||||
Returns a deep copy of the extensions mapping to prevent callers
|
||||
from accidentally mutating nested internal registry state.
|
||||
|
||||
Returns:
|
||||
Dictionary of extension_id -> metadata
|
||||
Dictionary of extension_id -> metadata (deep copies)
|
||||
"""
|
||||
return self.data["extensions"]
|
||||
return copy.deepcopy(self.data["extensions"])
|
||||
|
||||
def is_installed(self, extension_id: str) -> bool:
|
||||
"""Check if extension is installed.
|
||||
@@ -600,7 +656,7 @@ class ExtensionManager:
|
||||
result.append({
|
||||
"id": ext_id,
|
||||
"name": manifest.name,
|
||||
"version": metadata["version"],
|
||||
"version": metadata.get("version", "unknown"),
|
||||
"description": manifest.description,
|
||||
"enabled": metadata.get("enabled", True),
|
||||
"installed_at": metadata.get("installed_at"),
|
||||
@@ -1112,12 +1168,13 @@ class ExtensionCatalog:
|
||||
config_path: Path to extension-catalogs.yml
|
||||
|
||||
Returns:
|
||||
Ordered list of CatalogEntry objects, or None if file doesn't exist
|
||||
or contains no valid catalog entries.
|
||||
Ordered list of CatalogEntry objects, or None if file doesn't exist.
|
||||
|
||||
Raises:
|
||||
ValidationError: If any catalog entry has an invalid URL,
|
||||
the file cannot be parsed, or a priority value is invalid.
|
||||
the file cannot be parsed, a priority value is invalid,
|
||||
or the file exists but contains no valid catalog entries
|
||||
(fail-closed for security).
|
||||
"""
|
||||
if not config_path.exists():
|
||||
return None
|
||||
@@ -1129,12 +1186,17 @@ class ExtensionCatalog:
|
||||
)
|
||||
catalogs_data = data.get("catalogs", [])
|
||||
if not catalogs_data:
|
||||
return None
|
||||
# File exists but has no catalogs key or empty list - fail closed
|
||||
raise ValidationError(
|
||||
f"Catalog config {config_path} exists but contains no 'catalogs' entries. "
|
||||
f"Remove the file to use built-in defaults, or add valid catalog entries."
|
||||
)
|
||||
if not isinstance(catalogs_data, list):
|
||||
raise ValidationError(
|
||||
f"Invalid catalog config: 'catalogs' must be a list, got {type(catalogs_data).__name__}"
|
||||
)
|
||||
entries: List[CatalogEntry] = []
|
||||
skipped_entries: List[int] = []
|
||||
for idx, item in enumerate(catalogs_data):
|
||||
if not isinstance(item, dict):
|
||||
raise ValidationError(
|
||||
@@ -1142,6 +1204,7 @@ class ExtensionCatalog:
|
||||
)
|
||||
url = str(item.get("url", "")).strip()
|
||||
if not url:
|
||||
skipped_entries.append(idx)
|
||||
continue
|
||||
self._validate_catalog_url(url)
|
||||
try:
|
||||
@@ -1164,7 +1227,14 @@ class ExtensionCatalog:
|
||||
description=str(item.get("description", "")),
|
||||
))
|
||||
entries.sort(key=lambda e: e.priority)
|
||||
return entries if entries else None
|
||||
if not entries:
|
||||
# All entries were invalid (missing URLs) - fail closed for security
|
||||
raise ValidationError(
|
||||
f"Catalog config {config_path} contains {len(catalogs_data)} entries but none have valid URLs "
|
||||
f"(entries at indices {skipped_entries} were skipped). "
|
||||
f"Each catalog entry must have a 'url' field."
|
||||
)
|
||||
return entries
|
||||
|
||||
def get_active_catalogs(self) -> List[CatalogEntry]:
|
||||
"""Get the ordered list of active catalogs.
|
||||
|
||||
Reference in New Issue
Block a user