fix: prevent extension command shadowing#1994
Conversation
There was a problem hiding this comment.
Pull request overview
This PR closes a security/reliability gap in the extension system by adding install-time validation that prevents extensions from shadowing core Spec Kit commands or colliding with commands/aliases from other installed extensions.
Changes:
- Add install-time validation in
ExtensionManagerto reject core-namespace shadowing, invalid alias shapes, and collisions with installed extensions. - Expand extension installation tests to cover core-namespace conflicts, malformed aliases, and cross-extension collisions.
- Update extension docs/templates/examples to use namespaced aliases instead of legacy short aliases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions.py |
Introduces core-command reservation and install-time conflict validation for command/alias names. |
tests/test_extensions.py |
Adds/updates tests asserting the new validation behavior and updated alias naming. |
extensions/template/extension.yml |
Updates template manifest example to use namespaced aliases. |
extensions/RFC-EXTENSION-SYSTEM.md |
Updates examples away from short aliases; needs wording alignment for “backward compatibility”. |
extensions/EXTENSION-USER-GUIDE.md |
Updates user-facing examples to show namespaced alias usage. |
extensions/EXTENSION-DEVELOPMENT-GUIDE.md |
Updates developer guidance to show namespaced alias usage. |
extensions/EXTENSION-API-REFERENCE.md |
Updates manifest reference to note alias restrictions (needs to reflect all new constraints). |
Comments suppressed due to low confidence (1)
src/specify_cli/extensions.py:518
- The install-time validation only checks that names match the
speckit.<namespace>.<command>pattern, but it doesn’t enforce that<namespace>matchesmanifest.id. As written, an extension could publish commands/aliases under another extension’s namespace (namespace squatting/impersonation), and also block the real extension from installing later. Consider rejecting command/alias names where the parsed namespace (match group 1) is not exactlymanifest.id.
namespace = match.group(1)
if namespace in CORE_COMMAND_NAMES:
raise ValidationError(
f"{kind.capitalize()} '{name}' conflicts with core command namespace '{namespace}'"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. If not applicable, please explain why
Addressed in I fixed all 3 Copilot items:
Validation:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@afurm This does not prevent presets from overriding commands? |
Correct. This change does not prevent presets from overriding commands. The new validation only runs in the extension install path ( So the intent here is narrower: prevent extensions from claiming core-command namespaces or another extension’s namespace. It does not change the existing preset override mechanism. |
|
Thank you! |
|
Thank you!!! |
The upstream github#1994 added alias validation in _collect_manifest_command_names, which also rejected legacy 2-part alias names (e.g. 'speckit.verify'). Extend the same auto-correction logic from _validate() to cover aliases, so both 'speckit.command' and 'extension.command' alias formats are corrected to 'speckit.{ext_id}.{command}' with a compatibility warning instead of hard-failing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The upstream github#1994 added alias validation in _collect_manifest_command_names, which also rejected legacy 2-part alias names (e.g. 'speckit.verify'). Extend the same auto-correction logic from _validate() to cover aliases, so both 'speckit.command' and 'extension.command' alias formats are corrected to 'speckit.{ext_id}.{command}' with a compatibility warning instead of hard-failing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Relax alias validation in _collect_manifest_command_names() to only
enforce the 3-part speckit.{ext}.{cmd} pattern on primary command
names. Aliases retain type and duplicate checking but are otherwise
free-form, restoring pre-github#1994 behavior.
This unblocks community extensions (e.g. spec-kit-verify) that use
2-part aliases like 'speckit.verify'.
Fixes github#2110
) Relax alias validation in _collect_manifest_command_names() to only enforce the 3-part speckit.{ext}.{cmd} pattern on primary command names. Aliases retain type and duplicate checking but are otherwise free-form, restoring pre-#1994 behavior. This unblocks community extensions (e.g. spec-kit-verify) that use 2-part aliases like 'speckit.verify'. Fixes #2110
…e auto-correction (#2017) (#2027) * docs: warn about unofficial PyPI packages and recommend version verification (#1982) Clarify that only packages from github/spec-kit are official, and add `specify version` as a post-install verification step to help users catch accidental installation of an unrelated package with the same name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): auto-correct legacy command names instead of hard-failing (#2017) Community extensions that predate the strict naming requirement use two common legacy formats ('speckit.command' and 'extension.command'). Instead of rejecting them outright, auto-correct to the required 'speckit.{extension}.{command}' pattern and emit a compatibility warning so authors know they need to update their manifest. Names that cannot be safely corrected (e.g. single-segment names) still raise ValidationError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(tests): isolate preset catalog search test from community catalog network calls test_search_with_cached_data asserted exactly 2 results but was getting 4 because _get_merged_packs() queries the full built-in catalog stack (default + community). The community catalog had no local cache and hit the network, returning real presets. Writing a project-level preset-catalogs.yml that pins the test to the default URL only makes the count assertions deterministic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): extend auto-correction to aliases (#2017) The upstream #1994 added alias validation in _collect_manifest_command_names, which also rejected legacy 2-part alias names (e.g. 'speckit.verify'). Extend the same auto-correction logic from _validate() to cover aliases, so both 'speckit.command' and 'extension.command' alias formats are corrected to 'speckit.{ext_id}.{command}' with a compatibility warning instead of hard-failing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): address PR review feedback (#2017) - _try_correct_command_name: only correct 'X.Y' to 'speckit.ext_id.Y' when X matches ext_id, preventing misleading warnings followed by install failure due to namespace mismatch - _validate: add aliases type/string guards matching _collect_manifest _command_names defensive checks - _validate: track command renames and rewrite any hook.*.command references that pointed at a renamed command, emitting a warning - test: fix test_command_name_autocorrect_no_speckit_prefix to use ext_id matching the legacy namespace; add namespace-mismatch test - test: replace redundant preset-catalogs.yml isolation with monkeypatch.delenv("SPECKIT_PRESET_CATALOG_URL") so the env var cannot bypass catalog restriction in CI environments Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Update docs/installation.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(extensions): warn when hook command refs are silently canonicalized; fix grammar - Hook rewrites (alias-form or rename-map) now always emit a warning so extension authors know to update their manifests. Previously only rename-map rewrites produced a warning; pure alias-form lifts were silent. - Pluralize "command/commands" in the uninstall confirmation message so single-command extensions no longer print "1 commands". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): raise ValidationError for non-dict hook entries Silently skipping non-dict hook entries left them in manifest.hooks, causing HookExecutor.register_hooks() to crash with AttributeError when it called hook_config.get() on a non-mapping value. Also updates PR description to accurately reflect the implementation (no separate _try_correct_alias_name helper; aliases use the same _try_correct_command_name path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): derive remove cmd_count from registry, fix wording Previously cmd_count used len(ext_manifest.commands) which only counted primary commands and missed aliases. The registry's registered_commands already tracks every command name (primaries + aliases) per agent, so max(len(v) for v in registered_commands.values()) gives the correct total. Also changes "from AI agent" → "across AI agents" since remove() unregisters commands from all detected agents. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): distinguish missing vs empty registered_commands in remove prompt Using get() without a default lets us tell apart: - key missing (legacy registry entry) → fall back to manifest count - key present but empty dict (installed with no agent dirs) → show 0 Previously the truthiness check `if registered_commands and ...` treated both cases the same, so an empty dict fell back to len(manifest.commands) and overcounted commands that would actually be removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): clarify removal prompt wording to 'per agent' 'across AI agents' implied a total count, but cmd_count uses max() across agents (per-agent count). Using sum() would double-count since users think in logical commands, not per-agent files. 'per agent' accurately describes what the number represents. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): clarify cmd_count comment — per-agent max, not total The comment said 'covers all agents' implying a total, but cmd_count uses max() across agents (per-agent count). Updated comment to explain the max() choice and why sum() would double-count. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(extensions): add CLI tests for remove confirmation pluralization Adds TestExtensionRemoveCLI with two CliRunner tests: - singular: 1 registered command → '1 command per agent' - plural: 2 registered commands → '2 commands per agent' These prevent regressions on the cmd_count pluralization logic and the 'per agent' wording introduced in this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(agents): remove orphaned SKILL.md parent dirs on unregister For SKILL.md-based agents (codex, kimi), each command lives in its own subdirectory (e.g. .agents/skills/speckit-ext-cmd/SKILL.md). The previous unregister_commands() only unlinked the file, leaving an empty parent dir. Now attempts rmdir() on the parent when it differs from the agent commands dir. OSError is silenced so non-empty dirs (e.g. user files) are safely left. Adds test_unregister_skill_removes_parent_directory to cover this. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): drop alias pattern enforcement from _validate() Aliases are intentionally free-form to preserve community extension compatibility (e.g. 'speckit.verify' short aliases used by spec-kit-verify and other existing extensions). This aligns _validate() with the intent of upstream commit 4deb90f (fix: restore alias compatibility, #2110/#2125). Only type and None-normalization checks remain for aliases. Pattern enforcement continues for primary command names only. Updated tests to verify free-form aliases pass through unchanged with no warnings instead of being auto-corrected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(extensions): guard against non-dict command entries in _validate() If provides.commands contains a non-mapping entry (e.g. an int or string), 'name' not in cmd raises TypeError instead of a user-facing ValidationError. Added isinstance(cmd, dict) check at the top of the loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: iamaeroplane <michal.bachorik@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Fixes #1963.
This PR hardens extension installation so extensions cannot silently shadow core Spec Kit commands or collide with commands from other installed extensions.
Specifically, it:
speckit.*commandsspeckit.{extension}.{command}This is needed to close a security and reliability gap where an extension could overwrite core command files or another extension's command files during installation.
Testing
Ran
uv run specify --helpsuccessfully and confirmedextensioncommands are presentRan the full test suite successfully with
uv sync --extra test && uv run pytest(876 passed)Also ran
uv run --extra test pytest tests/test_extensions.pywhile developing the fixTested locally with
uv run specify --helpRan existing tests with
uv sync --extra test && uv run pytestTested with a sample project (not applicable for this change)
AI Disclosure
Used Codex to inspect the issue, implement the validation changes, update tests/docs, and run verification commands.