fix(cli): harden extension registration and discovery workflows#2499
Conversation
- Update memory-md from 0.7.9 to 0.8.0 - Update architecture-guard from 1.6.7 to 1.8.0
There was a problem hiding this comment.
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
installedlist maintenance toHookExecutor(register on hook registration; unregister on hook removal). - Introduce unit tests covering registration sorting/idempotency and initialization when
installedis missing. - Bump versions + download URLs (and
updated_at) for two entries inextensions/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.
|
Addressing feedback:
|
There was a problem hiding this comment.
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 callsregister_extension(), but the subsequent hook registration path still assumesconfigis a dict and thatconfig["hooks"]is a dict. Sinceget_project_config()can return any YAML type and existing configs could have a non-mappinghooksvalue, consider hardening here too (e.g., return defaults/coerce whenconfigorconfig["hooks"]isn’t a mapping) so installs don’t crash on corruptedextensions.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"] = {}
…ive unregister_hooks tests - Add dict guard to register_hooks() to handle corrupted extensions.yml (non-dict root) - Add 5 comprehensive tests for unregister_hooks() workflow: * Full workflow with hooks + installed list removal * Resilience when config has no 'hooks' key * Corrupted YAML handling * Multiple extension scenarios * All 11 tests passing
…le null hook values
- register_extension(): filter non-string entries from installed before sort
- register_hooks(): normalize hooks to {} when missing or not a dict
- unregister_hooks(): add isinstance(config, dict) guard before key checks
- unregister_hooks(): coerce null/scalar hook lists to [] before iteration
- tests: add 3 regression tests for no-hooks manifest, mixed-type installed, null hook values
- All 14 tests passing
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/specify_cli/extensions.py:2601
register_hooks()assumesconfig["hooks"][hook_name]is a list of dicts. Ifextensions.ymlis partially corrupted/hand-edited (e.g.,hooks: {after_tasks: null}or an existing hook list containing non-dict entries), theexisting = [...] if h.get(...)comprehension and later update loop will raise (TypeErroron iterating non-list /AttributeErroron.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 whenconfig["installed"]is already a list. Ifextensions.ymlhasinstalled: null/string (or a list with non-string values), uninstalling viaunregister_hooks()will leave the installed marker behind and won’t repair the invalid shape. Consider normalizinginstalledhere the same way asregister_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
|
Please address Copilot feedback |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/specify_cli/extensions.py:2728
unregister_hooks()always writesextensions.ymleven when no hooks are removed/normalized (e.g., extension has no hooks, or config already has no matching entries). This can cause unnecessary YAML churn and overwrite user formatting. Consider tracking whether any changes were made (removal, coercion, cleanup) and only callingsave_project_config()when the config actually changed.
# Always remove from installed list (Feedback from review)
self.unregister_extension(extension_id)
config = self.get_project_config()
if not isinstance(config, dict):
config = {}
# We don't save yet, as there are no hooks to unregister,
# but unregister_extension above might have already saved a normalized config.
return
if "hooks" not in config or not isinstance(config["hooks"], dict):
return
# Remove hooks for this extension
for hook_name in list(config["hooks"].keys()):
hook_list = config["hooks"][hook_name]
if not isinstance(hook_list, list):
config["hooks"][hook_name] = []
continue
config["hooks"][hook_name] = [
h
for h in hook_list
if isinstance(h, dict) and h.get("extension") != extension_id
]
# Clean up empty hook arrays
config["hooks"] = {
name: hooks for name, hooks in config["hooks"].items() if hooks
}
self.save_project_config(config)
- Files reviewed: 4/4 changed files
- Comments generated: 2
mnriem
left a comment
There was a problem hiding this comment.
Please address Copiot feedback
…then hook unregister assertion
…clarify test comment
|
Thank you! |
This PR hardens the extension registration process in the Spec Kit CLI.
Key Changes
HookExecutorandExtensionManagerto automatically maintain aninstalledlist in.specify/extensions.yml, ensuring robust extension discovery without requiring separate template changes.tests/test_extension_registration.pyandtests/test_extension_update_hardening.pycovering corrupted config, rollback, and edge cases.