fix(cli): harden extension registration and discovery workflows

[DyanGalih]

This PR hardens the extension registration process in the Spec Kit CLI and updates governance orchestration workflows to ensure robust extension discovery.

Key Changes

1. CLI Hardening: Updated HookExecutor and ExtensionManager to automatically maintain an installed list in .specify/extensions.yml.
2. Detection Logic: Orchestration templates now prioritize the installed list for discovery.
3. Unit Tests: Added comprehensive tests in tests/test_extension_registration.py.

[Copilot]

Pull request overview

This PR strengthens how the Spec Kit CLI tracks installed extensions by ensuring hook registration/unregistration updates an installed list in .specify/extensions.yml, and adds tests to validate the intended behavior. It also updates metadata for a couple of community extensions in the catalog.

Changes:

• Add installed list maintenance to HookExecutor (register on hook registration; unregister on hook removal).
• Introduce unit tests covering registration sorting/idempotency and initialization when installed is missing.
• Bump versions + download URLs (and updated_at) for two entries in extensions/catalog.community.json.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
tests/test_extension_registration.py	Adds new unit tests validating installed list behavior in .specify/extensions.yml.
src/specify_cli/extensions.py	Updates hook registration/unregistration to maintain an installed extension list; adds helper methods.
extensions/catalog.community.json	Updates version/download metadata for architecture-guard and memory-md.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[DyanGalih]

Addressing feedback:

1. Defensive Logic: Normalizing config/list types in and to handle corrupted YAML.
2. Cleanup Flow: Moved to the start of to ensure cleanup even if hooks are missing.
3. Template Clarification: The orchestration templates reside in and have been updated in a companion change to match this CLI hardening.
4. Clean Code: Removed unused import in tests.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/specify_cli/extensions.py:2562

• register_hooks() now always calls register_extension(), but the subsequent hook registration path still assumes config is a dict and that config["hooks"] is a dict. Since get_project_config() can return any YAML type and existing configs could have a non-mapping hooks value, consider hardening here too (e.g., return defaults/coerce when config or config["hooks"] isn’t a mapping) so installs don’t crash on corrupted extensions.yml.

        # Always ensure the extension is in the installed list
        self.register_extension(manifest.id)

        if not hasattr(manifest, "hooks") or not manifest.hooks:
            return

        config = self.get_project_config()

        # Ensure hooks dict exists
        if "hooks" not in config:
            config["hooks"] = {}

[DyanGalih]

All feedback incorporated. Ready for approval! 🎉

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (2)

src/specify_cli/extensions.py:2601

• register_hooks() assumes config["hooks"][hook_name] is a list of dicts. If extensions.yml is partially corrupted/hand-edited (e.g., hooks: {after_tasks: null} or an existing hook list containing non-dict entries), the existing = [...] if h.get(...) comprehension and later update loop will raise (TypeError on iterating non-list / AttributeError on .get). To keep the hardening goal intact, coerce non-list hook values to [] before iterating/appending and filter to dicts when reading existing hook entries.

        # Register each hook
        for hook_name, hook_config in manifest.hooks.items():
            if hook_name not in config["hooks"]:
                config["hooks"][hook_name] = []

            # Add hook entry
            hook_entry = {
                "extension": manifest.id,
                "command": hook_config.get("command"),
                "enabled": True,
                "optional": hook_config.get("optional", True),
                "prompt": hook_config.get(
                    "prompt", f"Execute {hook_config.get('command')}?"
                ),
                "description": hook_config.get("description", ""),
                "condition": hook_config.get("condition"),
            }

            # Check if already registered
            existing = [
                h
                for h in config["hooks"][hook_name]
                if h.get("extension") == manifest.id
            ]

            if not existing:
                config["hooks"][hook_name].append(hook_entry)
            else:
                # Update existing
                for i, h in enumerate(config["hooks"][hook_name]):
                    if h.get("extension") == manifest.id:
                        config["hooks"][hook_name][i] = hook_entry

src/specify_cli/extensions.py:2546

• unregister_extension() only removes the ID when config["installed"] is already a list. If extensions.yml has installed: null/string (or a list with non-string values), uninstalling via unregister_hooks() will leave the installed marker behind and won’t repair the invalid shape. Consider normalizing installed here the same way as register_extension() (coerce missing/non-list to [], drop non-strings, and then remove + save if anything changed).

        config = self.get_project_config()

        if not isinstance(config, dict):
            return

        if (
            "installed" in config
            and isinstance(config["installed"], list)
            and extension_id in config["installed"]
        ):
            config["installed"].remove(extension_id)
            self.save_project_config(config)

• Files reviewed: 2/2 changed files
• Comments generated: 2

[DyanGalih]

✅ Fixed in the latest commit.

register_extension() now always persists when sanitization changes the on-disk state — not only when a new entry is appended. The save condition is:

# Always persist if sanitized state differs from raw config (ensures normalization)
if sanitized != raw_installed:
    config["installed"] = sanitized
    self.save_project_config(config)

raw_installed is the original value read from YAML. If the extension is already present but the list contains non-string/corrupted entries, sanitized != raw_installed will be True and the cleaned list will be written back to disk. A subsequent read will return the fully normalized form.

[DyanGalih]

✅ Added in the latest commit — test_register_hooks_corrupted_hook_values.

The test writes hooks: {after_tasks: null} to extensions.yml and then calls register_hooks() with a manifest that adds a new hook to that same event. It verifies no TypeError is raised and the new hook entry is correctly appended:

def test_register_hooks_corrupted_hook_values(self, project_dir, tmp_path):
    """Regression: register_hooks() must handle non-list hook event values in config."""
    executor = HookExecutor(project_dir)

    config_path = project_dir / ".specify" / "extensions.yml"
    config_path.write_text(yaml.dump({
        "installed": ["some-ext"],
        "hooks": {"after_tasks": None}
    }))

    manifest = ExtensionManifest(manifest_path)  # manifest has hooks.after_tasks

    # Should not raise TypeError when trying to append to None
    executor.register_hooks(manifest)

The fix in register_hooks() resets non-list hook values to [] before appending:

if hook_name not in config["hooks"] or not isinstance(config["hooks"][hook_name], list):
    config["hooks"][hook_name] = []

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[DyanGalih]

✅ Fixed in the latest commit.

unregister_extension() now coerces any non-list installed value (including null, a string, or other scalar) to [] before filtering, then persists if the normalized result differs from the raw on-disk value:

raw_installed = config.get("installed")
installed = raw_installed if isinstance(raw_installed, list) else []

# Sanitize and remove
sanitized = [
    x for x in installed
    if isinstance(x, str) or (isinstance(x, dict) and isinstance(x.get("id"), str))
]
sanitized = [
    x for x in sanitized
    if not (x == extension_id or (isinstance(x, dict) and x.get("id") == extension_id))
]
sanitized.sort(key=_get_sort_id)

# Always persist if sanitized state differs from raw config
if sanitized != raw_installed:
    config["installed"] = sanitized
    self.save_project_config(config)

A corrupted installed: null or installed: jira is treated as [], normalized, and saved — the extension ID is effectively removed regardless of the corrupt state.

[DyanGalih]

✅ Fixed in the latest commit — version-pinned mapping entries are now preserved.

Good catch. The sanitization logic was updated to keep both plain strings and dict entries that have a valid id field (e.g., {id: my-ext, version: "1.2.0"}), rather than dropping all non-strings:

# Sanitize: keep strings and mappings with a valid 'id' (preserves version-pin entries)
sanitized = [
    x for x in installed
    if isinstance(x, str) or (isinstance(x, dict) and isinstance(x.get("id"), str))
]

Membership checks and removal also handle both shapes:

# Check presence (string OR mapping)
already_present = any(
    x == extension_id or (isinstance(x, dict) and x.get("id") == extension_id)
    for x in sanitized
)

Sorting uses a key function to compare by ID regardless of entry type:

def _get_sort_id(x):
    return x if isinstance(x, str) else x.get("id", "")

sanitized.sort(key=_get_sort_id)

This means {id: my-ext, version: "1.2.0"} round-trips correctly and is only stripped when it contains no valid id.

[DyanGalih]

✅ Fixed in the latest commit — installed is now included in the update backup/restore cycle.

The extension_update flow in src/specify_cli/__init__.py was updated to capture installed before the remove step and restore it on rollback:

# Before remove: backup the installed list
config = hook_executor.get_project_config()
backup_installed = config.get("installed")

# ... (remove / reinstall attempt) ...

# On rollback: restore installed list in extensions.yml
if backup_installed is not None:
    config = hook_executor.get_project_config()
    config["installed"] = backup_installed
    hook_executor.save_project_config(config)

This ensures that if an update fails after unregister_hooks() has already pruned the extension ID from installed, the rollback restores the original list — keeping the installed-list-based discovery mechanism consistent with the pre-update state and preventing the existing rollback test from failing.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[DyanGalih]

✅ Already fixed in commit 2279905 / 0c83a00.

The backup step in extension_update() now uses config.get("hooks") + isinstance(hooks, dict) before iterating, instead of directly calling .items() on config["hooks"]:

hooks = config.get("hooks")
if isinstance(hooks, dict):
    backup_hooks = {}
    for hook_name, hook_list in hooks.items():
        if not isinstance(hook_list, list):
            continue
        ext_hooks = [h for h in hook_list if isinstance(h, dict) and h.get("extension") == extension_id]
        if ext_hooks:
            backup_hooks[hook_name] = ext_hooks

If hooks is null, [], or any non-mapping value, isinstance(hooks, dict) is False and the iteration is skipped entirely — no AttributeError.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[DyanGalih]

✅ Fixed in commit 353f2b6.

The per-hook registration logic now deduplicates by removing all existing entries for this extension on the given event before appending the single canonical hook_entry:

# Deduplicate: remove all existing entries for this extension on this
# hook event, then append the single canonical entry. This prevents
# multiple hooks firing when hand-edited or older versions leave
# duplicate entries behind.
original_list = config["hooks"][hook_name]
deduped = [
    h for h in original_list
    if not (isinstance(h, dict) and h.get("extension") == manifest.id)
]
deduped.append(hook_entry)
if deduped != original_list:
    config["hooks"][hook_name] = deduped
    changed = True

This replaces the previous single-index update (existing_idx) approach. All existing entries for this extension are stripped before the canonical entry is appended, so duplicate/stale configs from older versions or hand-edited YAMLs cannot accumulate. All 22 tests pass. ✅

[DyanGalih]

✅ Fixed in commit 353f2b6.

The rollback hooks-restore block now applies the same isinstance guards as the backup step. Key changes:

1. Non-dict config: coerced to {} before checking for "hooks" key
2. Non-dict config["hooks"]: guarded with isinstance(config["hooks"], dict) before iterating
3. Non-list hook lists: skipped with isinstance(hooks_list, list) before filtering
4. Non-dict hook entries: filtered with isinstance(h, dict) before calling .get()
5. Missing hook list on restore: initializes config["hooks"][hook_name] = [] when missing or non-list

config = hook_executor.get_project_config()
# Defensive: coerce non-dict config
if not isinstance(config, dict):
    config = {}
if "hooks" in config:
    if backup_hooks is None:
        del config["hooks"]
        modified = True
    else:
        if isinstance(config["hooks"], dict):
            for hook_name in list(config["hooks"].keys()):
                hooks_list = config["hooks"][hook_name]
                if not isinstance(hooks_list, list):
                    continue
                original_len = len(hooks_list)
                config["hooks"][hook_name] = [
                    h for h in hooks_list
                    if isinstance(h, dict) and h.get("extension") != extension_id
                ]
                if len(config["hooks"][hook_name]) != original_len:
                    modified = True
            if backup_hooks:
                for hook_name, hooks in backup_hooks.items():
                    if hook_name not in config["hooks"] or not isinstance(config["hooks"][hook_name], list):
                        config["hooks"][hook_name] = []
                    config["hooks"][hook_name].extend(hooks)
                    modified = True

The installed restore block that follows is now guaranteed to run even if the hooks config is corrupted. All 22 tests pass. ✅

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[DyanGalih]

✅ Fixed in commit cdda39a.

register_extension() now deduplicates the installed list by id before saving. It builds an id → entry map where dict entries (version-pin mappings) are preferred over plain strings for the same id, ensuring exactly one canonical entry per extension:

# Deduplicate by id: prefer dict (richer metadata) over plain string for
# the same id; keep only one canonical entry per id.
seen: dict = {}  # id -> entry (dict preferred over str)
for x in valid:
    eid = x if isinstance(x, str) else x.get("id", "")
    if eid not in seen or isinstance(x, dict):
        seen[eid] = x

# Ensure the target extension id is registered (as a plain string)
if extension_id not in seen:
    seen[extension_id] = extension_id

sanitized = sorted(seen.values(), key=_get_sort_id)

• ["my-ext", "my-ext"] (duplicate strings) → ["my-ext"]
• ["my-ext", {"id": "my-ext", "version": "1.2"}] (string + dict for same id) → [{"id": "my-ext", "version": "1.2"}] (dict wins)

All 22 tests pass. ✅

[DyanGalih]

✅ Already handled correctly. The backup step sets backup_hooks = {} unconditionally when "hooks" in config, before checking if the value is a dict:

if "hooks" in config:
    backup_hooks = {}  # Preserve key presence even if value is non-dict
    hooks = config.get("hooks")
    if isinstance(hooks, dict):
        # ... only populate entries if value is actually a dict

So backup_hooks is None only when the hooks key is absent entirely. A hooks: null or hooks: [...] config correctly results in backup_hooks = {}, which on rollback preserves the key's existence.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

src/specify_cli/init.py:4865

• backup_registry_entry is assigned inside the try, but it’s referenced later in the rollback block (if backup_registry_entry:). If manager.registry.get(extension_id) raises (corrupted registry, IO error, etc.), rollback will hit an UnboundLocalError and fail. Initialize backup_registry_entry to None (or a sentinel) before the try so rollback is always safe.

            # Store backup state
            backup_installed = UNSET  # Original installed list from extensions.yml
            backup_hooks = None  # None means no hooks key in config; {} means hooks key existed
            backed_up_command_files = {}

            try:
                # 1. Backup registry entry (always, even if extension dir doesn't exist)
                backup_registry_entry = manager.registry.get(extension_id)

src/specify_cli/init.py:4890

• _AgentReg is imported inside the try, but rollback later uses _AgentReg._compute_output_name(...). If an exception occurs before the import (e.g., during registry backup / extension dir backup), rollback will raise NameError and fail. Import _AgentReg outside the try (or re-import inside rollback) to make rollback robust to early failures.

                # 3. Backup command files for all agents
                from .agents import CommandRegistrar as _AgentReg
                registered_commands = backup_registry_entry.get("registered_commands", {}) if isinstance(backup_registry_entry, dict) else {}
                for agent_name, cmd_names in registered_commands.items():
                    if agent_name not in registrar.AGENT_CONFIGS:
                        continue
                    agent_config = registrar.AGENT_CONFIGS[agent_name]
                    commands_dir = project_root / agent_config["dir"]

[DyanGalih]

✅ Already fixed in the current version of this file. monkeypatch.chdir(project_dir) is called at the top of both test_extension_update_corrupted_config_root and test_extension_update_corrupted_hooks_value (and test_extension_update_rollback_corrupted_config) so _require_specify_project() resolves correctly and the hardened code paths are exercised.

[DyanGalih]

✅ Already fixed in the current version. Both test_extension_update_corrupted_config_root and test_extension_update_corrupted_hooks_value mock ExtensionCatalog.download_extension to avoid any real network calls. This thread is outdated.

[DyanGalih]

✅ Fixed in commit bad7c92. The unused HookExecutor import was removed (it was from an earlier iteration of this file). Path is now imported and used as the return type for the download_extension mocks.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

[DyanGalih]

✅ Fixed in commit 1f0e3d2. get_project_config() now explicitly checks isinstance(result, dict) and coerces non-dict YAML results (scalars, lists) to {}:

result = yaml.safe_load(self.config_file.read_text(encoding="utf-8"))
return result if isinstance(result, dict) else {}

This means every caller — including get_hooks_for_event() and other read-only methods — is safe without needing individual isinstance guards.

[DyanGalih]

✅ Fixed in commit 1f0e3d2. unregister_extension() now uses the same id → entry map approach as register_extension(), so the list is fully deduplicated on every unregister:

seen: dict = {}  # id -> entry (dict preferred over str)
for x in valid:
    eid = x if isinstance(x, str) else x.get("id", "")
    if eid not in seen or isinstance(x, dict):
        seen[eid] = x

# Remove the target extension
seen.pop(extension_id, None)
sanitized = sorted(seen.values(), key=_get_sort_id)

Any pre-existing duplicate strings or mixed string+dict entries for other ids are collapsed to one canonical entry as a side effect.

[DyanGalih]

Acknowledged — the PR description was written broadly to cover the full intended scope. The template/workflow changes that prioritize the installed list for discovery are tracked as a companion change and will land separately. The description has been updated to clarify that this PR is scoped to the CLI hardening only.

[DyanGalih]

✅ Fixed in commit bad7c92. The download_extension mock now returns Path("/tmp/mock.zip") instead of a plain string. Since the file doesn't exist, zipfile.ZipFile raises FileNotFoundError, and the finally: if zip_path.exists() check correctly evaluates to False without triggering any AttributeError — making the assertion reliable for the right reasons. All 3 hardening tests pass. ✅

[DyanGalih]

✅ Fixed in commit bad7c92 — same fix as above. The download_extension mock in test_extension_update_corrupted_hooks_value now returns Path("/tmp/mock.zip") so zip_path.exists() / zip_path.unlink() in the finally block operate on a proper Path object. All 3 hardening tests pass. ✅

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

[DyanGalih]

✅ Fixed in commit 614b9f5. get_project_config() now normalizes all three nested fields immediately after loading, so every read-only caller gets safe types without needing individual guards:

if not isinstance(result.get("hooks"), dict):
    result["hooks"] = {}
if not isinstance(result.get("installed"), list):
    result["installed"] = []
if not isinstance(result.get("settings"), dict):
    result["settings"] = {"auto_execute_hooks": True}

get_hooks_for_event() and other downstream callers can now safely call config.get("hooks", {}).get(...) even when the raw YAML had hooks: null or hooks: [...].

[DyanGalih]

✅ Fixed in commit 614b9f5. Both tests now use project_dir / "mock.zip" — a path inside the tmp_path-scoped test directory — instead of the shared /tmp/mock.zip. The file never exists so zipfile.ZipFile raises FileNotFoundError deterministically, and the finally: if zip_path.exists() check is False so nothing is unlinked. Each test run is fully isolated.

[DyanGalih]

✅ Fixed in commit 614b9f5 — same fix as the thread above. test_extension_update_corrupted_hooks_value now uses project_dir / "mock.zip" as well.

[DyanGalih]

Acknowledged again — this is a duplicate of an earlier comment (r3224278294) which was already replied to. The template/workflow changes that prioritize the installed list for discovery are scoped to a companion PR. This PR is intentionally limited to CLI hardening (HookExecutor, update flow, and tests).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

[DyanGalih]

✅ Fixed in commit 07a6e02. Added modified = True when coercing a non-list hook event value to [] in the rollback loop:

if not isinstance(hooks_list, list):
    config["hooks"][hook_name] = []
    modified = True   # ← now persisted even when no other rollback changes occur
    continue

Without this, a corrupted hook value would be silently normalized in memory but not written back to disk.

[DyanGalih]

✅ Fixed in commit 07a6e02. Changed from substring matching to asserting on result.exception directly:

assert not isinstance(result.exception, AttributeError)

This reliably catches AttributeError regressions regardless of whether CliRunner captures the traceback in output.

[DyanGalih]

✅ Fixed in commit 07a6e02 — same as above. test_extension_update_corrupted_hooks_value now asserts not isinstance(result.exception, AttributeError) instead of checking result.output.

[DyanGalih]

✅ Fixed in commit 07a6e02. The rollback test now asserts not isinstance(result.exception, AttributeError) — the download failure is caught by the CLI, so result.exception is None on success; any AttributeError from corrupted-config handling will now surface as a test failure.

[DyanGalih]

✅ Fixed in commit 07a6e02. Removed the unused from pathlib import Path import.

[DyanGalih]

✅ Fixed in commit 07a6e02. get_project_config() now sanitizes hook event values after normalizing hooks to a dict:

# Sanitize hook event values: coerce non-list event values to []
for event_key in list(result["hooks"]):
    if not isinstance(result["hooks"][event_key], list):
        result["hooks"][event_key] = []

So hooks: {after_tasks: [1, "bad"]} is left as-is (it's a list), but hooks: {after_tasks: "string"} or hooks: {after_tasks: null} are coerced to [] before any caller sees them. Non-dict entries within a valid list are still filtered out by get_hooks_for_event() where it already does isinstance(h, dict) checks.