Skip to content

fix: prevent extension command shadowing#1994

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

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

Conversation

@afurm

@afurm afurm commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

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

afurm commented Mar 27, 2026

Copy link
Copy Markdown
Contributor Author

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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

mnriem commented Mar 27, 2026

Copy link
Copy Markdown
Collaborator

@afurm This does not prevent presets from overriding commands?

@afurm

afurm commented Mar 27, 2026

Copy link
Copy Markdown
Contributor Author

@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

mnriem commented Mar 27, 2026

Copy link
Copy Markdown
Collaborator

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

4 participants