Skip to content

fix: prevent extension command shadowing#1994

Merged
mnriem merged 3 commits intogithub:mainfrom
afurm:af/fix-extension-command-collisions
Mar 27, 2026
Merged

fix: prevent extension command shadowing#1994
mnriem merged 3 commits intogithub:mainfrom
afurm:af/fix-extension-command-collisions

Conversation

@afurm
Copy link
Copy Markdown
Contributor

@afurm afurm commented Mar 27, 2026

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:

  • adds install-time validation for extension command and alias names
  • rejects extension IDs and command namespaces that overlap with core speckit.* commands
  • rejects malformed short aliases that do not follow speckit.{extension}.{command}
  • rejects command/alias collisions with already-installed extensions
  • updates extension tests and shipped extension docs/examples to use namespaced aliases

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 --help successfully and confirmed extension commands are present

  • Ran 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.py while developing the fix

  • Tested locally with uv run specify --help

  • Ran existing tests with uv sync --extra test && uv run pytest

  • Tested with a sample project (not applicable for this change)

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

Used Codex to inspect the issue, implement the validation changes, update tests/docs, and run verification commands.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ExtensionManager to 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> matches manifest.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 exactly manifest.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.

Comment thread extensions/EXTENSION-API-REFERENCE.md Outdated
Comment thread extensions/RFC-EXTENSION-SYSTEM.md Outdated
Comment thread src/specify_cli/extensions.py Outdated
Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback. If not applicable, please explain why

@afurm
Copy link
Copy Markdown
Contributor Author

afurm commented Mar 27, 2026

Please address Copilot feedback. If not applicable, please explain why

Addressed in 3571f75.

I fixed all 3 Copilot items:

  • Enforced extension.id namespace ownership.
  • Replaced the hard-coded core command list with discovery from bundled templates, plus a parity test.
  • Updated the docs/RFC wording to match the implemented validation and migration behavior.

Validation:

  • uv run --with '.[test]' pytest tests/test_extensions.py -q
  • 148 passed

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/specify_cli/extensions.py
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Mar 27, 2026

@afurm This does not prevent presets from overriding commands?

@afurm
Copy link
Copy Markdown
Contributor Author

afurm commented Mar 27, 2026

@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 (ExtensionManager.install_from_directory() / install_from_zip() in src/specify_cli/extensions.py). Presets still use the separate preset registration flow in src/specify_cli/presets.py, where command overrides are applied by priority and restored on removal.

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.

@mnriem mnriem merged commit 796b4f4 into github:main Mar 27, 2026
12 checks passed
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Mar 27, 2026

Thank you!

@afurm afurm deleted the af/fix-extension-command-collisions branch March 27, 2026 15:57
@mbachorik
Copy link
Copy Markdown
Contributor

Thank you!!!

mbachorik pushed a commit to mbachorik/spec-kit that referenced this pull request Mar 30, 2026
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>
mbachorik pushed a commit to mbachorik/spec-kit that referenced this pull request Apr 4, 2026
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>
mnriem added a commit to mnriem/spec-kit that referenced this pull request Apr 8, 2026
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
mnriem added a commit that referenced this pull request Apr 8, 2026
)

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
mnriem pushed a commit that referenced this pull request Apr 15, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extension commands can shadow core spec-kit commands

4 participants