Skip to content

Commit 50936c9

Browse files
iamaeroplaneclaude
andcommitted
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>
1 parent 6fb6214 commit 50936c9

File tree

1 file changed

+54
-57
lines changed

1 file changed

+54
-57
lines changed

src/specify_cli/__init__.py

Lines changed: 54 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1805,19 +1805,21 @@ def _resolve_installed_extension(
18051805
argument: str,
18061806
installed_extensions: list,
18071807
command_name: str = "command",
1808-
) -> tuple[str, str]:
1808+
allow_not_found: bool = False,
1809+
) -> tuple[Optional[str], Optional[str]]:
18091810
"""Resolve an extension argument (ID or display name) to an installed extension.
18101811
18111812
Args:
18121813
argument: Extension ID or display name provided by user
18131814
installed_extensions: List of installed extension dicts from manager.list_installed()
18141815
command_name: Name of the command for error messages (e.g., "enable", "disable")
1816+
allow_not_found: If True, return (None, None) when not found instead of raising
18151817
18161818
Returns:
1817-
Tuple of (extension_id, display_name)
1819+
Tuple of (extension_id, display_name), or (None, None) if allow_not_found=True and not found
18181820
18191821
Raises:
1820-
typer.Exit: If extension not found or name is ambiguous
1822+
typer.Exit: If extension not found (and allow_not_found=False) or name is ambiguous
18211823
"""
18221824
from rich.table import Table
18231825

@@ -1850,6 +1852,8 @@ def _resolve_installed_extension(
18501852
raise typer.Exit(1)
18511853
else:
18521854
# No match by ID or display name
1855+
if allow_not_found:
1856+
return (None, None)
18531857
console.print(f"[red]Error:[/red] Extension '{argument}' is not installed")
18541858
raise typer.Exit(1)
18551859

@@ -2434,38 +2438,10 @@ def extension_info(
24342438
installed = manager.list_installed()
24352439

24362440
# Try to resolve from installed extensions first (by ID or name)
2437-
resolved_installed_id = None
2438-
resolved_installed_name = None
2439-
2440-
# First check for exact ID match
2441-
for ext in installed:
2442-
if ext["id"] == extension:
2443-
resolved_installed_id = ext["id"]
2444-
resolved_installed_name = ext["name"]
2445-
break
2446-
2447-
# If no ID match, check for name matches (with ambiguity detection)
2448-
if not resolved_installed_id:
2449-
name_matches = [ext for ext in installed if ext["name"].lower() == extension.lower()]
2450-
if len(name_matches) == 1:
2451-
resolved_installed_id = name_matches[0]["id"]
2452-
resolved_installed_name = name_matches[0]["name"]
2453-
elif len(name_matches) > 1:
2454-
from rich.table import Table
2455-
console.print(
2456-
f"[red]Error:[/red] Extension name '{extension}' is ambiguous. "
2457-
"Multiple installed extensions share this name:"
2458-
)
2459-
table = Table(title="Matching extensions")
2460-
table.add_column("ID", style="cyan", no_wrap=True)
2461-
table.add_column("Name", style="white")
2462-
table.add_column("Version", style="green")
2463-
for ext in name_matches:
2464-
table.add_row(ext.get("id", ""), ext.get("name", ""), str(ext.get("version", "")))
2465-
console.print(table)
2466-
console.print(f"\nPlease rerun using the extension ID:")
2467-
console.print(f" [bold]specify extension info <extension-id>[/bold]")
2468-
raise typer.Exit(1)
2441+
# Use allow_not_found=True since the extension may be catalog-only
2442+
resolved_installed_id, resolved_installed_name = _resolve_installed_extension(
2443+
extension, installed, "info", allow_not_found=True
2444+
)
24692445

24702446
# Try catalog lookup (with error handling)
24712447
# If we resolved an installed extension by display name, use its ID for catalog lookup
@@ -2688,6 +2664,7 @@ def extension_update(
26882664
updates_available.append(
26892665
{
26902666
"id": ext_id,
2667+
"name": ext_info.get("name", ext_id), # Display name for status messages
26912668
"installed": str(installed_version),
26922669
"available": str(catalog_version),
26932670
"download_url": ext_info.get("download_url"),
@@ -2722,7 +2699,7 @@ def extension_update(
27222699

27232700
for update in updates_available:
27242701
extension_id = update["id"]
2725-
ext_name = update["id"]
2702+
ext_name = update["name"] # Use display name for user-facing messages
27262703
console.print(f"📦 Updating {ext_name}...")
27272704

27282705
# Backup paths
@@ -2732,7 +2709,7 @@ def extension_update(
27322709

27332710
# Store backup state
27342711
backup_registry_entry = None
2735-
backup_hooks = {} # Initialize to empty dict, not None
2712+
backup_hooks = None # None means no hooks key in config; {} means hooks key existed
27362713
backed_up_command_files = {}
27372714

27382715
try:
@@ -2773,9 +2750,11 @@ def extension_update(
27732750
backed_up_command_files[str(prompt_file)] = str(backup_prompt_path)
27742751

27752752
# 4. Backup hooks from extensions.yml
2753+
# Use backup_hooks=None to indicate config had no "hooks" key (don't create on restore)
2754+
# Use backup_hooks={} to indicate config had "hooks" key with no hooks for this extension
27762755
config = hook_executor.get_project_config()
27772756
if "hooks" in config:
2778-
backup_hooks = {}
2757+
backup_hooks = {} # Config has hooks key - preserve this fact
27792758
for hook_name, hook_list in config["hooks"].items():
27802759
ext_hooks = [h for h in hook_list if h.get("extension") == extension_id]
27812760
if ext_hooks:
@@ -2785,12 +2764,25 @@ def extension_update(
27852764
zip_path = catalog.download_extension(extension_id)
27862765
try:
27872766
# 6. Validate extension ID from ZIP BEFORE modifying installation
2767+
# Handle both root-level and nested extension.yml (GitHub auto-generated ZIPs)
27882768
with zipfile.ZipFile(zip_path, "r") as zf:
2789-
try:
2769+
import yaml
2770+
manifest_data = None
2771+
namelist = zf.namelist()
2772+
2773+
# First try root-level extension.yml
2774+
if "extension.yml" in namelist:
27902775
with zf.open("extension.yml") as f:
2791-
import yaml
27922776
manifest_data = yaml.safe_load(f) or {}
2793-
except KeyError:
2777+
else:
2778+
# Look for extension.yml in a single top-level subdirectory
2779+
# (e.g., "repo-name-branch/extension.yml")
2780+
manifest_paths = [n for n in namelist if n.endswith("/extension.yml") and n.count("/") == 1]
2781+
if len(manifest_paths) == 1:
2782+
with zf.open(manifest_paths[0]) as f:
2783+
manifest_data = yaml.safe_load(f) or {}
2784+
2785+
if manifest_data is None:
27942786
raise ValueError("Downloaded extension archive is missing 'extension.yml'")
27952787

27962788
zip_extension_id = manifest_data.get("extension", {}).get("id")
@@ -2803,7 +2795,7 @@ def extension_update(
28032795
manager.remove(extension_id, keep_config=True)
28042796

28052797
# 8. Install new version
2806-
installed_manifest = manager.install_from_zip(zip_path, speckit_version)
2798+
_ = manager.install_from_zip(zip_path, speckit_version)
28072799

28082800
# 9. Restore enabled state from backup
28092801
# If extension was disabled before update, disable it again
@@ -2861,26 +2853,31 @@ def extension_update(
28612853
shutil.copy2(backup_file, original_file)
28622854

28632855
# Restore hooks in extensions.yml
2864-
# Always remove any hooks for this extension (from failed install),
2865-
# then restore backed-up hooks (even if empty)
2866-
if backup_hooks is not None:
2867-
config = hook_executor.get_project_config()
2868-
if "hooks" not in config:
2869-
config["hooks"] = {}
2870-
2871-
# First remove any existing hooks for this extension across all hook groups
2856+
# - backup_hooks=None means original config had no "hooks" key
2857+
# - backup_hooks={} or {...} means config had hooks key
2858+
config = hook_executor.get_project_config()
2859+
if "hooks" in config:
2860+
# Remove any hooks for this extension added by failed install
2861+
modified = False
28722862
for hook_name, hooks_list in config["hooks"].items():
2863+
original_len = len(hooks_list)
28732864
config["hooks"][hook_name] = [
28742865
h for h in hooks_list
28752866
if h.get("extension") != extension_id
28762867
]
2877-
2878-
# Then add back the backed up hooks (may be empty)
2879-
for hook_name, hooks in backup_hooks.items():
2880-
if hook_name not in config["hooks"]:
2881-
config["hooks"][hook_name] = []
2882-
config["hooks"][hook_name].extend(hooks)
2883-
hook_executor.save_project_config(config)
2868+
if len(config["hooks"][hook_name]) != original_len:
2869+
modified = True
2870+
2871+
# Add back the backed up hooks if any
2872+
if backup_hooks:
2873+
for hook_name, hooks in backup_hooks.items():
2874+
if hook_name not in config["hooks"]:
2875+
config["hooks"][hook_name] = []
2876+
config["hooks"][hook_name].extend(hooks)
2877+
modified = True
2878+
2879+
if modified:
2880+
hook_executor.save_project_config(config)
28842881

28852882
# Restore registry entry (use restore() since entry was removed)
28862883
if backup_registry_entry:

0 commit comments

Comments
 (0)