mirror of
https://github.com/github/spec-kit.git
synced 2026-03-23 22:03:08 +00:00
feat(extensions,presets): add priority-based resolution ordering (#1855)
* feat(extensions,presets): add priority-based resolution ordering Add priority field to extension and preset registries for deterministic template resolution when multiple sources provide the same template. Extensions: - Add `list_by_priority()` method to ExtensionRegistry - Add `--priority` option to `extension add` command - Add `extension set-priority` command - Show priority in `extension list` and `extension info` - Preserve priority during `extension update` - Update RFC documentation Presets: - Add `preset set-priority` command - Show priority in `preset info` output - Use priority ordering in PresetResolver for extensions Both systems: - Lower priority number = higher precedence (default: 10) - Backwards compatible with legacy entries (missing priority defaults to 10) - Comprehensive test coverage including backwards compatibility Closes #1845 Closes #1854 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address code review feedback - list_by_priority(): add secondary sort by ID for deterministic ordering, return deep copies to prevent mutation - install_from_directory/zip: validate priority >= 1 early - extension add CLI: validate --priority >= 1 before install - PresetRegistry.update(): preserve installed_at timestamp - Test assertions: use exact source string instead of substring match Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address additional review feedback - PresetResolver: add fallback to directory scanning when registry is empty/corrupted for robustness and backwards compatibility - PresetRegistry.update(): add guard to prevent injecting installed_at when absent in existing entry (mirrors ExtensionRegistry behavior) - RFC: update extension list example to match actual CLI output format Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: restore defensive code and RFC descriptions lost in rebase - Restore defensive code in list_by_priority() with .get() and isinstance check - Restore detailed --from URL and --dev option descriptions in RFC Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add defensive code to presets list_by_priority() - Add .get() and isinstance check for corrupted/empty registry - Move copy import to module level (remove local import) - Matches defensive pattern used in extensions.py Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address reviewer feedback on priority resolution - Rename _normalize_priority to normalize_priority (public API) - Add comprehensive tests for normalize_priority function (9 tests) - Filter non-dict metadata entries in list_by_priority() methods - Fix extension priority resolution to merge registered and unregistered extensions into unified sorted list (unregistered get implicit priority 10) - Add tests for extension priority resolution ordering (4 tests) The key fix ensures unregistered extensions with implicit priority 10 correctly beat registered extensions with priority > 10, and vice versa. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: DRY refactor and strengthen test assertions - Extract _get_all_extensions_by_priority() helper in PresetResolver to eliminate duplicated extension list construction - Add priority=10 assertion to test_legacy_extension_without_priority_field - Add priority=10 assertion to test_legacy_preset_without_priority_field Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add isinstance(dict) checks for corrupted registry entries Add defensive checks throughout CLI commands and manager methods to handle cases where registry entries may be corrupted (non-dict values). This prevents AttributeError when calling .get() on non-dict metadata. Locations fixed: - __init__.py: preset/extension info, set-priority, enable/disable, upgrade commands - extensions.py: list_installed() - presets.py: list_installed() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: normalize priority display to match resolution behavior Use normalize_priority() for all priority display in CLI commands to ensure displayed values match actual resolution behavior when registry data is corrupted/hand-edited. Locations fixed: - extensions.py: list_installed() - presets.py: list_installed(), PresetResolver - __init__.py: preset info, extension info, set-priority commands Also added GraphQL query for unresolved PR comments to CLAUDE.md. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: repair corrupted priority values in set-priority commands Changed set-priority commands to check if the raw stored value is already a valid int equal to the requested priority before skipping. This ensures corrupted values (e.g., "high") get repaired even when setting to the default priority (10). Also removed CLAUDE.md that was accidentally added to the repo. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: harden registry update methods against corrupted entries - Normalize priority when restoring during extension update to prevent propagating corrupted values (e.g., "high", 0, negative) - Add isinstance(dict) checks in ExtensionRegistry.update() and PresetRegistry.update() to handle corrupted entries (string/list) that would cause TypeError on merge Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use safe fallback for version in list_installed() When registry entry is corrupted (non-dict), metadata becomes {} after the isinstance check. Use metadata.get("version", manifest.version) instead of metadata["version"] to avoid KeyError. 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
@@ -7,6 +7,7 @@ Presets are self-contained, versioned collections of templates
|
||||
customize the Spec-Driven Development workflow.
|
||||
"""
|
||||
|
||||
import copy
|
||||
import json
|
||||
import hashlib
|
||||
import os
|
||||
@@ -23,6 +24,8 @@ import yaml
|
||||
from packaging import version as pkg_version
|
||||
from packaging.specifiers import SpecifierSet, InvalidSpecifier
|
||||
|
||||
from .extensions import ExtensionRegistry, normalize_priority
|
||||
|
||||
|
||||
@dataclass
|
||||
class PresetCatalogEntry:
|
||||
@@ -271,6 +274,38 @@ class PresetRegistry:
|
||||
del self.data["presets"][pack_id]
|
||||
self._save()
|
||||
|
||||
def update(self, pack_id: str, updates: dict):
|
||||
"""Update preset metadata in registry.
|
||||
|
||||
Merges the provided updates with the existing entry, preserving any
|
||||
fields not specified. The installed_at timestamp is always preserved
|
||||
from the original entry.
|
||||
|
||||
Args:
|
||||
pack_id: Preset ID
|
||||
updates: Partial metadata to merge into existing metadata
|
||||
|
||||
Raises:
|
||||
KeyError: If preset is not installed
|
||||
"""
|
||||
if pack_id not in self.data["presets"]:
|
||||
raise KeyError(f"Preset '{pack_id}' not found in registry")
|
||||
existing = self.data["presets"][pack_id]
|
||||
# Handle corrupted registry entries (e.g., string/list instead of dict)
|
||||
if not isinstance(existing, dict):
|
||||
existing = {}
|
||||
# Merge: existing fields preserved, new fields override
|
||||
merged = {**existing, **updates}
|
||||
# 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["presets"][pack_id] = merged
|
||||
self._save()
|
||||
|
||||
def get(self, pack_id: str) -> Optional[dict]:
|
||||
"""Get preset metadata from registry.
|
||||
|
||||
@@ -294,14 +329,26 @@ class PresetRegistry:
|
||||
"""Get all installed presets sorted by priority.
|
||||
|
||||
Lower priority number = higher precedence (checked first).
|
||||
Presets with equal priority are sorted alphabetically by ID
|
||||
for deterministic ordering.
|
||||
|
||||
Returns:
|
||||
List of (pack_id, metadata) tuples sorted by priority
|
||||
List of (pack_id, metadata_copy) tuples sorted by priority.
|
||||
Metadata is deep-copied to prevent accidental mutation.
|
||||
"""
|
||||
packs = self.data["presets"]
|
||||
packs = self.data.get("presets", {}) or {}
|
||||
if not isinstance(packs, dict):
|
||||
packs = {}
|
||||
sortable_packs = []
|
||||
for pack_id, meta in packs.items():
|
||||
if not isinstance(meta, dict):
|
||||
continue
|
||||
metadata_copy = copy.deepcopy(meta)
|
||||
metadata_copy["priority"] = normalize_priority(metadata_copy.get("priority", 10))
|
||||
sortable_packs.append((pack_id, metadata_copy))
|
||||
return sorted(
|
||||
packs.items(),
|
||||
key=lambda item: item[1].get("priority", 10),
|
||||
sortable_packs,
|
||||
key=lambda item: (item[1]["priority"], item[0]),
|
||||
)
|
||||
|
||||
def is_installed(self, pack_id: str) -> bool:
|
||||
@@ -680,9 +727,13 @@ class PresetManager:
|
||||
Installed preset manifest
|
||||
|
||||
Raises:
|
||||
PresetValidationError: If manifest is invalid
|
||||
PresetValidationError: If manifest is invalid or priority is invalid
|
||||
PresetCompatibilityError: If pack is incompatible
|
||||
"""
|
||||
# Validate priority
|
||||
if priority < 1:
|
||||
raise PresetValidationError("Priority must be a positive integer (1 or higher)")
|
||||
|
||||
manifest_path = source_dir / "preset.yml"
|
||||
manifest = PresetManifest(manifest_path)
|
||||
|
||||
@@ -729,14 +780,19 @@ class PresetManager:
|
||||
Args:
|
||||
zip_path: Path to preset ZIP file
|
||||
speckit_version: Current spec-kit version
|
||||
priority: Resolution priority (lower = higher precedence, default 10)
|
||||
|
||||
Returns:
|
||||
Installed preset manifest
|
||||
|
||||
Raises:
|
||||
PresetValidationError: If manifest is invalid
|
||||
PresetValidationError: If manifest is invalid or priority is invalid
|
||||
PresetCompatibilityError: If pack is incompatible
|
||||
"""
|
||||
# Validate priority early
|
||||
if priority < 1:
|
||||
raise PresetValidationError("Priority must be a positive integer (1 or higher)")
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
temp_path = Path(tmpdir)
|
||||
|
||||
@@ -808,6 +864,9 @@ class PresetManager:
|
||||
result = []
|
||||
|
||||
for pack_id, metadata in self.registry.list().items():
|
||||
# Ensure metadata is a dictionary to avoid AttributeError when using .get()
|
||||
if not isinstance(metadata, dict):
|
||||
metadata = {}
|
||||
pack_dir = self.presets_dir / pack_id
|
||||
manifest_path = pack_dir / "preset.yml"
|
||||
|
||||
@@ -816,13 +875,13 @@ class PresetManager:
|
||||
result.append({
|
||||
"id": pack_id,
|
||||
"name": manifest.name,
|
||||
"version": metadata["version"],
|
||||
"version": metadata.get("version", manifest.version),
|
||||
"description": manifest.description,
|
||||
"enabled": metadata.get("enabled", True),
|
||||
"installed_at": metadata.get("installed_at"),
|
||||
"template_count": len(manifest.templates),
|
||||
"tags": manifest.tags,
|
||||
"priority": metadata.get("priority", 10),
|
||||
"priority": normalize_priority(metadata.get("priority")),
|
||||
})
|
||||
except PresetValidationError:
|
||||
result.append({
|
||||
@@ -834,7 +893,7 @@ class PresetManager:
|
||||
"installed_at": metadata.get("installed_at"),
|
||||
"template_count": 0,
|
||||
"tags": [],
|
||||
"priority": metadata.get("priority", 10),
|
||||
"priority": normalize_priority(metadata.get("priority")),
|
||||
})
|
||||
|
||||
return result
|
||||
@@ -1393,6 +1452,40 @@ class PresetResolver:
|
||||
self.overrides_dir = self.templates_dir / "overrides"
|
||||
self.extensions_dir = project_root / ".specify" / "extensions"
|
||||
|
||||
def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]:
|
||||
"""Build unified list of registered and unregistered extensions sorted by priority.
|
||||
|
||||
Registered extensions use their stored priority; unregistered directories
|
||||
get implicit priority=10. Results are sorted by (priority, ext_id) for
|
||||
deterministic ordering.
|
||||
|
||||
Returns:
|
||||
List of (priority, ext_id, metadata_or_none) tuples sorted by priority.
|
||||
"""
|
||||
if not self.extensions_dir.exists():
|
||||
return []
|
||||
|
||||
registry = ExtensionRegistry(self.extensions_dir)
|
||||
registered_extensions = registry.list_by_priority()
|
||||
registered_extension_ids = {ext_id for ext_id, _ in registered_extensions}
|
||||
|
||||
all_extensions: list[tuple[int, str, dict | None]] = []
|
||||
|
||||
for ext_id, metadata in registered_extensions:
|
||||
priority = normalize_priority(metadata.get("priority") if metadata else None)
|
||||
all_extensions.append((priority, ext_id, metadata))
|
||||
|
||||
# Add unregistered directories with implicit priority=10
|
||||
for ext_dir in self.extensions_dir.iterdir():
|
||||
if not ext_dir.is_dir() or ext_dir.name.startswith("."):
|
||||
continue
|
||||
if ext_dir.name not in registered_extension_ids:
|
||||
all_extensions.append((10, ext_dir.name, None))
|
||||
|
||||
# Sort by (priority, ext_id) for deterministic ordering
|
||||
all_extensions.sort(key=lambda x: (x[0], x[1]))
|
||||
return all_extensions
|
||||
|
||||
def resolve(
|
||||
self,
|
||||
template_name: str,
|
||||
@@ -1445,18 +1538,18 @@ class PresetResolver:
|
||||
if candidate.exists():
|
||||
return candidate
|
||||
|
||||
# Priority 3: Extension-provided templates
|
||||
if self.extensions_dir.exists():
|
||||
for ext_dir in sorted(self.extensions_dir.iterdir()):
|
||||
if not ext_dir.is_dir() or ext_dir.name.startswith("."):
|
||||
continue
|
||||
for subdir in subdirs:
|
||||
if subdir:
|
||||
candidate = ext_dir / subdir / f"{template_name}{ext}"
|
||||
else:
|
||||
candidate = ext_dir / "templates" / f"{template_name}{ext}"
|
||||
if candidate.exists():
|
||||
return candidate
|
||||
# Priority 3: Extension-provided templates (sorted by priority — lower number wins)
|
||||
for _priority, ext_id, _metadata in self._get_all_extensions_by_priority():
|
||||
ext_dir = self.extensions_dir / ext_id
|
||||
if not ext_dir.is_dir():
|
||||
continue
|
||||
for subdir in subdirs:
|
||||
if subdir:
|
||||
candidate = ext_dir / subdir / f"{template_name}{ext}"
|
||||
else:
|
||||
candidate = ext_dir / f"{template_name}{ext}"
|
||||
if candidate.exists():
|
||||
return candidate
|
||||
|
||||
# Priority 4: Core templates
|
||||
if template_type == "template":
|
||||
@@ -1514,17 +1607,24 @@ class PresetResolver:
|
||||
except ValueError:
|
||||
continue
|
||||
|
||||
if self.extensions_dir.exists():
|
||||
for ext_dir in sorted(self.extensions_dir.iterdir()):
|
||||
if not ext_dir.is_dir() or ext_dir.name.startswith("."):
|
||||
continue
|
||||
try:
|
||||
resolved.relative_to(ext_dir)
|
||||
for _priority, ext_id, ext_meta in self._get_all_extensions_by_priority():
|
||||
ext_dir = self.extensions_dir / ext_id
|
||||
if not ext_dir.is_dir():
|
||||
continue
|
||||
try:
|
||||
resolved.relative_to(ext_dir)
|
||||
if ext_meta:
|
||||
version = ext_meta.get("version", "?")
|
||||
return {
|
||||
"path": resolved_str,
|
||||
"source": f"extension:{ext_dir.name}",
|
||||
"source": f"extension:{ext_id} v{version}",
|
||||
}
|
||||
except ValueError:
|
||||
continue
|
||||
else:
|
||||
return {
|
||||
"path": resolved_str,
|
||||
"source": f"extension:{ext_id} (unregistered)",
|
||||
}
|
||||
except ValueError:
|
||||
continue
|
||||
|
||||
return {"path": resolved_str, "source": "core"}
|
||||
|
||||
Reference in New Issue
Block a user