fix(extensions): address final reviewer comments (#2017)

- Reject aliases whose SKILL output name would collide with the primary
  command (e.g. primary 'speckit.myext.run' + alias 'myext.run' both map
  to skill 'speckit-myext-run')
- Fix hook rewrite warning to show the final canonical value instead of
  the intermediate rename_map value; compute final_ref (rename + lift)
  first, then write to hook_data and emit warning in a single step
- Guard registered_commands against non-dict in extension_remove CLI
  (corrupted/legacy registry entries no longer crash with AttributeError)
- Update alias tests to use non-colliding alias suffix ('greet' not 'hello')
- Add test_alias_collision_with_primary_skill_name_rejected

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: iamaeroplane

src/specify_cli/__init__.py

@@ -3124,8 +3124,11 @@ def extension_remove(
 
     # Get extension info for command and skill counts
     reg_meta = manager.registry.get(extension_id)
-    # Count total registered commands across all agents (includes aliases)
-    registered_commands = reg_meta.get("registered_commands", {}) if reg_meta else {}
+    # Count total registered commands across all agents (includes aliases).
+    # Normalize to dict in case registry entry is corrupted or from an older version.
+    registered_commands = reg_meta.get("registered_commands") if reg_meta else None
+    if not isinstance(registered_commands, dict):
+        registered_commands = {}
     cmd_count = sum(
         len(names) for names in registered_commands.values()
         if isinstance(names, list)

src/specify_cli/extensions.py

@@ -229,7 +229,21 @@ def _validate(self):
                     )
                 alias_match = EXTENSION_ALIAS_PATTERN.match(alias)
                 if alias_match and alias_match.group(1) != 'speckit':
-                    pass  # already valid: 'myext.command' form
+                    # Valid 'ext.cmd' form — check it won't collide with the primary
+                    # command's SKILL output name. For SKILL.md agents,
+                    # _compute_output_name strips 'speckit.' so e.g. primary
+                    # 'speckit.myext.run' and alias 'myext.run' both map to
+                    # 'speckit-myext-run', causing the alias write to overwrite
+                    # the primary skill file.
+                    primary = cmd["name"]
+                    if primary.startswith("speckit.") and primary[len("speckit."):] == alias:
+                        skill_name = "speckit-" + alias.replace(".", "-")
+                        raise ValidationError(
+                            f"Alias '{alias}' would collide with primary command "
+                            f"'{primary}' on SKILL.md-based agents (both map to "
+                            f"'{skill_name}'). "
+                            f"Choose a distinct alias name."
+                        )
                 else:
                     corrected = self._try_correct_alias_name(alias, ext["id"])
                     if corrected:
@@ -263,23 +277,24 @@ def _validate(self):
                 continue
 
             # Step 1: apply rename_map (command was auto-corrected during validation).
-            rewritten = rename_map.get(command_ref, command_ref)
-            if rewritten != command_ref:
-                hook_data["command"] = rewritten
-                self.warnings.append(
-                    f"Hook '{hook_name}' referenced renamed command '{command_ref}'; "
-                    f"updated to '{rewritten}'. "
-                    f"The extension author should update the manifest."
-                )
-                command_ref = rewritten
+            final_ref = rename_map.get(command_ref, command_ref)
 
             # Step 2: if the ref is in alias form '{ext_id}.cmd', lift it to
             # 'speckit.{ext_id}.cmd' so skill-mode hook invocation renders correctly.
-            parts = command_ref.split('.')
+            parts = final_ref.split('.')
             if len(parts) == 2 and parts[0] == ext["id"]:
                 canonical = f"speckit.{ext['id']}.{parts[1]}"
                 if EXTENSION_COMMAND_NAME_PATTERN.match(canonical):
-                    hook_data["command"] = canonical
+                    final_ref = canonical
+
+            if final_ref != command_ref:
+                hook_data["command"] = final_ref
+                if command_ref in rename_map:
+                    self.warnings.append(
+                        f"Hook '{hook_name}' referenced renamed command '{command_ref}'; "
+                        f"updated to '{final_ref}'. "
+                        f"The extension author should update the manifest."
+                    )
 
     @staticmethod
     def _try_correct_command_name(name: str, ext_id: str) -> Optional[str]:

tests/test_extensions.py

@@ -305,63 +305,85 @@ def test_command_name_mismatched_namespace_not_corrected(self, temp_dir, valid_m
             ExtensionManifest(manifest_path)
 
     def test_alias_valid_two_part_no_prefix(self, temp_dir, valid_manifest_data):
-        """Test that a 'test-ext.hello' alias is accepted as-is with no warning."""
+        """Test that a 'test-ext.greet' alias is accepted as-is with no warning."""
         import yaml
 
-        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["test-ext.hello"]
+        # Use a name distinct from the primary command's suffix ('hello') to avoid
+        # the SKILL.md output-name collision (both would map to speckit-test-ext-hello).
+        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["test-ext.greet"]
 
         manifest_path = temp_dir / "extension.yml"
         with open(manifest_path, 'w') as f:
             yaml.dump(valid_manifest_data, f)
 
         manifest = ExtensionManifest(manifest_path)
 
-        assert manifest.commands[0]["aliases"] == ["test-ext.hello"]
+        assert manifest.commands[0]["aliases"] == ["test-ext.greet"]
         assert manifest.warnings == []
 
     def test_alias_autocorrect_speckit_two_part(self, temp_dir, valid_manifest_data):
         """Test that legacy 'speckit.command' alias is corrected to '{ext_id}.command'."""
         import yaml
 
-        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["speckit.hello"]
+        # Use 'speckit.greet' → 'test-ext.greet' to avoid colliding with the primary
+        # command's SKILL output name (speckit.test-ext.hello → speckit-test-ext-hello).
+        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["speckit.greet"]
 
         manifest_path = temp_dir / "extension.yml"
         with open(manifest_path, 'w') as f:
             yaml.dump(valid_manifest_data, f)
 
         manifest = ExtensionManifest(manifest_path)
 
-        assert manifest.commands[0]["aliases"] == ["test-ext.hello"]
+        assert manifest.commands[0]["aliases"] == ["test-ext.greet"]
         assert len(manifest.warnings) == 1
-        assert "speckit.hello" in manifest.warnings[0]
-        assert "test-ext.hello" in manifest.warnings[0]
+        assert "speckit.greet" in manifest.warnings[0]
+        assert "test-ext.greet" in manifest.warnings[0]
 
     def test_alias_autocorrect_speckit_three_part(self, temp_dir, valid_manifest_data):
         """Test that a 3-part 'speckit.ext.command' alias is corrected to 'ext.command'."""
         import yaml
 
         # Clear hooks so the alias rename doesn't also trigger a hook-reference warning.
+        # Use 'speckit.test-ext.greet' → 'test-ext.greet' to avoid colliding with the
+        # primary command's SKILL output name (speckit.test-ext.hello → speckit-test-ext-hello).
         valid_manifest_data.pop("hooks", None)
-        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["speckit.test-ext.hello"]
+        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["speckit.test-ext.greet"]
 
         manifest_path = temp_dir / "extension.yml"
         with open(manifest_path, 'w') as f:
             yaml.dump(valid_manifest_data, f)
 
         manifest = ExtensionManifest(manifest_path)
 
-        assert manifest.commands[0]["aliases"] == ["test-ext.hello"]
+        assert manifest.commands[0]["aliases"] == ["test-ext.greet"]
         assert len(manifest.warnings) == 1
-        assert "speckit.test-ext.hello" in manifest.warnings[0]
-        assert "test-ext.hello" in manifest.warnings[0]
+        assert "speckit.test-ext.greet" in manifest.warnings[0]
+        assert "test-ext.greet" in manifest.warnings[0]
+
+    def test_alias_collision_with_primary_skill_name_rejected(self, temp_dir, valid_manifest_data):
+        """Alias whose SKILL output name matches the primary command's is rejected."""
+        import yaml
+
+        # Primary 'speckit.test-ext.hello' maps to skill 'speckit-test-ext-hello'.
+        # Alias 'test-ext.hello' would map to the same skill name — collision.
+        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["test-ext.hello"]
+
+        manifest_path = temp_dir / "extension.yml"
+        with open(manifest_path, "w") as f:
+            yaml.dump(valid_manifest_data, f)
+
+        with pytest.raises(ValidationError, match="would collide with primary command"):
+            ExtensionManifest(manifest_path)
 
     def test_hook_alias_ref_canonicalized_to_speckit_form(self, temp_dir, valid_manifest_data):
         """Hook command refs in alias form are lifted to canonical speckit.ext.cmd form."""
         import yaml
 
-        # Manifest uses a valid alias but the hook mistakenly references the alias name.
-        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["test-ext.hello"]
-        valid_manifest_data["hooks"]["after_tasks"]["command"] = "test-ext.hello"
+        # Manifest uses a valid alias (distinct suffix to avoid SKILL name collision)
+        # but the hook mistakenly references the alias name instead of the primary.
+        valid_manifest_data["provides"]["commands"][0]["aliases"] = ["test-ext.greet"]
+        valid_manifest_data["hooks"]["after_tasks"]["command"] = "test-ext.greet"
 
         manifest_path = temp_dir / "extension.yml"
         with open(manifest_path, "w") as f:
@@ -370,7 +392,7 @@ def test_hook_alias_ref_canonicalized_to_speckit_form(self, temp_dir, valid_mani
         manifest = ExtensionManifest(manifest_path)
 
         # Hook ref should be lifted to canonical form for skill-mode invocation.
-        assert manifest.hooks["after_tasks"]["command"] == "speckit.test-ext.hello"
+        assert manifest.hooks["after_tasks"]["command"] == "speckit.test-ext.greet"
 
     def test_hook_non_dict_value_raises(self, temp_dir, valid_manifest_data):
         """A non-mapping hooks value raises ValidationError."""