Skip to content

fix: unofficial PyPI warning (#1982) and legacy extension command name auto-correction (#2017)#2027

Open
mbachorik wants to merge 10 commits intogithub:mainfrom
mbachorik:fix/pypi-warning-and-legacy-extension-names
Open

fix: unofficial PyPI warning (#1982) and legacy extension command name auto-correction (#2017)#2027
mbachorik wants to merge 10 commits intogithub:mainfrom
mbachorik:fix/pypi-warning-and-legacy-extension-names

Conversation

@mbachorik
Copy link
Copy Markdown
Contributor

@mbachorik mbachorik commented Mar 30, 2026

Summary

Changes

`docs/installation.md` / `README.md`

  • Warning block clarifying GitHub is the only official source
  • `specify version` verification step added after install instructions

`src/specify_cli/extensions.py`

  • `ExtensionManifest._validate()`: auto-corrects legacy 2-part primary command names (`{ext_id}.cmd` or `speckit.cmd` → `speckit.{ext_id}.cmd`), emitting compatibility warnings; names that cannot be safely corrected still raise `ValidationError`
  • Alias names follow the same correction path via `_try_correct_command_name()` — no separate alias helper
  • Hook `command` refs are rewritten when they point at a renamed command; alias-form refs (`{ext_id}.cmd`) are also canonicalized to `speckit.{ext_id}.cmd`; a warning is emitted whenever any rewrite occurs
  • Individual hook entries validated as dicts — non-mapping entries raise `ValidationError` instead of silently skipping (which would crash `HookExecutor.register_hooks()` later)
  • SKILL output-name collision detection: a `seen_skill_names` dict is maintained across all commands and aliases (including auto-corrected aliases); any duplicate raises `ValidationError`
  • `_try_correct_command_name()`: static helper covering both `speckit.cmd` and `{ext_id}.cmd` legacy forms for primary commands and aliases

`src/specify_cli/init.py`

  • `extension_add`: prints compatibility warnings from `manifest.warnings` after successful install
  • `extension_remove` confirmation: correct singular/plural (`1 command` vs `N commands`)

`src/specify_cli/agents.py`

  • `CommandRegistrar.unregister_commands()`: removes empty skill parent directories after unlinking SKILL.md files, preventing orphaned `.claude/skills/speckit-*/` dirs

`tests/test_extensions.py`

  • `TestExtensionManifest`: new tests covering primary/alias auto-correction, hook ref rewriting and canonicalization, collision detection, non-dict hook entry validation
  • `TestCommandRegistrar`: `test_unregister_skill_removes_parent_directory` covering orphaned dir cleanup
  • `TestExtensionRemoveCLI`: CLI tests covering singular/plural command count in removal confirmation

`tests/test_presets.py`

  • `test_search_with_cached_data`: patched to use `unittest.mock.patch` on `get_active_catalogs` to isolate test from live community catalog network calls

Test plan

Unit tests

  • 154 extension tests pass (pytest tests/test_extensions.py)
  • Full suite passes

Live integration tests (real CLI, real filesystem, temp project dir)

All scenarios run against the workspace CLI with actual specify extension add/remove invocations:

Well-formed extension (foobar test extension):

✓ Extension installed successfully!
✓ .specify/extensions/foobar/ exists
✓ 11 command files created in .claude/commands/
✓ No compatibility warnings emitted

Legacy 2-part command name (foobar.hellospeckit.foobar.hello):

✓ Extension installed successfully!
⚠  Compatibility warning: Command name 'foobar.hello' does not follow the required
   pattern 'speckit.{extension}.{command}'. Registering as 'speckit.foobar.hello'.
   The extension author should update the manifest to use this name.
✓ Command file registered under canonical name

Hook with alias-form ref (foobar.hellospeckit.foobar.hello):

✓ Extension installed successfully!
⚠  Compatibility warning: Hook 'after_tasks' referenced command 'foobar.hello';
   updated to canonical form 'speckit.foobar.hello'. The extension author should
   update the manifest.

Non-dict hook entry (install must fail):

Validation Error: Hook 'after_tasks' must be a mapping, got str
✓ Extension not installed (install correctly aborted)

Remove — singular (1 command):

⚠  This will remove:
   • 1 command from AI agent

Remove — plural (2 commands):

⚠  This will remove:
   • 2 commands from AI agent

🤖 Generated with Claude Code

@mbachorik mbachorik requested a review from mnriem as a code owner March 30, 2026 21:25
Copilot AI review requested due to automatic review settings March 30, 2026 21:25
…ication (github#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>
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 improves user safety and extension compatibility by warning against unofficial PyPI packages, adding a post-install verification step, and making legacy community extension command names auto-correct (with warnings) instead of hard-failing. It also stabilizes a previously flaky preset-catalog test by preventing unintended live community catalog lookups.

Changes:

  • Add prominent documentation warnings that Spec Kit’s official packages are only distributed from github/spec-kit, plus a recommended specify version verification step.
  • In extension manifest validation, auto-correct common legacy 2-part command names to the required 3-part speckit.{extension}.{command} form and surface compatibility warnings on install.
  • Make TestPresetCatalog::test_search_with_cached_data deterministic by restricting catalogs to default-only via a project-level preset-catalogs.yml.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
README.md Adds an “official source only” warning and a specify version verification step after install instructions.
docs/installation.md Adds the same warning and a verification section recommending specify version.
src/specify_cli/extensions.py Adds command-name regex constant, manifest warnings, and auto-correction logic for legacy command names.
src/specify_cli/__init__.py Prints manifest compatibility warnings after successful specify extension add.
tests/test_extensions.py Adds tests for both auto-correction paths and the no-warning valid path; updates an existing docstring.
tests/test_presets.py Writes .specify/preset-catalogs.yml in a test to avoid community-catalog network flakiness.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iamaeroplane and others added 2 commits March 30, 2026 23:31
…iling (github#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>
… 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>
@mbachorik mbachorik force-pushed the fix/pypi-warning-and-legacy-extension-names branch from 56c6b66 to 44d1996 Compare March 30, 2026 21:33
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>
Copilot AI review requested due to automatic review settings March 30, 2026 21:47
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 6 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

- _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>
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 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mnriem mnriem self-requested a review March 31, 2026 19:55
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

@mbachorik
Copy link
Copy Markdown
Contributor Author

@mnriem I've bundled this with the fix for 2/3dot alias notation. Will wait for feedback from the reporter (or we decide tonight and agree on fix). Will address all copilot feedback as usual .. (switching it to draft so it does not spam you)

@mbachorik mbachorik marked this pull request as draft April 1, 2026 11:27
@mbachorik mbachorik force-pushed the fix/pypi-warning-and-legacy-extension-names branch from 3fed7f7 to caee3e0 Compare April 4, 2026 07:34
Copilot AI review requested due to automatic review settings April 4, 2026 07:34
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 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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 6 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings April 4, 2026 15:29
@mbachorik
Copy link
Copy Markdown
Contributor Author

mbachorik commented Apr 4, 2026

Ok, seems we're ready. Plenty of edge cases.

@mnriem there's another issue (unrelated but I suspect it might have impact) with support for Claude (moving commands to skills). hit that today (running latest spec-kit) and have isolated fix in my workspace. will open PR after this one ...

@mbachorik mbachorik requested a review from mnriem April 4, 2026 20:26
@mbachorik mbachorik force-pushed the fix/pypi-warning-and-legacy-extension-names branch from b0019c5 to 16f03ff Compare April 6, 2026 08:58
@mbachorik mbachorik requested a review from Copilot April 6, 2026 09:59
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 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 6, 2026 10:04
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 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mbachorik mbachorik marked this pull request as draft April 6, 2026 10:17
iamaeroplane and others added 2 commits April 6, 2026 12:52
…ed; 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>
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>
Copilot AI review requested due to automatic review settings April 6, 2026 11:06
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 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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>
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 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…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>
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 6 out of 6 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.

@mbachorik mbachorik marked this pull request as ready for review April 6, 2026 12:38
@mbachorik
Copy link
Copy Markdown
Contributor Author

@mnriem happy easter. ready again.

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 improves installation safety messaging (warns about unofficial PyPI packages) and restores compatibility for community extensions by auto-correcting legacy command/alias names while emitting warnings instead of hard-failing, plus it stabilizes a previously flaky preset-catalog test.

Changes:

  • Add prominent GitHub-only package warnings and a specify version verification step to install docs.
  • Extend extension manifest validation to auto-correct certain legacy command/alias names and rewrite hook command references with compatibility warnings.
  • Update CLI UX for extension install/remove output and patch a preset-catalog test to avoid live network calls.
Show a summary per file
File Description
README.md Adds official-source warning + post-install specify version verification step.
docs/installation.md Adds GitHub-only warning and verification guidance.
src/specify_cli/extensions.py Implements command/alias auto-correction + hook reference rewriting + warning accumulation.
src/specify_cli/__init__.py Prints manifest compatibility warnings on install; improves extension-remove command count text.
tests/test_extensions.py Adds coverage for auto-correction and hook validation behavior changes.
tests/test_presets.py Removes env override and patches catalog selection to prevent network flakiness.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 1

Comment on lines +3720 to +3723
cmd_count = max(
(len(v) for v in registered_commands.values() if isinstance(v, list)),
default=0,
)
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

cmd_count is computed using max(len(v) ...) across agents, but the removal prompt says "across AI agents". If registered_commands differs per agent (possible if registration partially succeeded or agent dirs changed), max can undercount the total files that will be removed. Consider either summing across agents for an "across agents" total, or clarifying the wording as "up to N commands per agent" if max is intentional to avoid double-counting.

Copilot uses AI. Check for mistakes.
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

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.

3 participants