Skip to content

fix(extensions): restore core-command discovery broken by #3014 module move#3292

Closed
dhruv-15-03 wants to merge 1 commit into
github:mainfrom
dhruv-15-03:fix/3274-extensions-core-command-discovery
Closed

fix(extensions): restore core-command discovery broken by #3014 module move#3292
dhruv-15-03 wants to merge 1 commit into
github:mainfrom
dhruv-15-03:fix/3274-extensions-core-command-discovery

Conversation

@dhruv-15-03

@dhruv-15-03 dhruv-15-03 commented Jul 1, 2026

Copy link
Copy Markdown

What

Fix the off-by-one path resolution in specify_cli.extensions._load_core_command_names() so it actually discovers the bundled core command templates instead of silently returning the hardcoded fallback set. Fixes #3294.

Why

When this code moved from src/specify_cli/extensions.py to src/specify_cli/extensions/__init__.py (#3014, 826e193), the file went one directory deeper but the bespoke Path(__file__) parent counts were not updated. Both candidate dirs now resolve to non-existent paths:

  • wheel -> specify_cli/extensions/core_pack/commands (real: specify_cli/core_pack/commands)
  • source -> src/templates/commands (real: repo-root templates/commands)

So _load_core_command_names() always falls through to _FALLBACK_CORE_COMMAND_NAMES. It only looks healthy because the fallback currently equals the 10 real command stems. CORE_COMMAND_NAMES (built from this function) guards extensions against shadowing core commands (#1994); with discovery dead, that guard depends on the fallback being hand-maintained -- converge was manually added to it in #3001 (0c29d89).

Same off-by-one class @mnriem diagnosed for the presets loader (re #3086, #2826), but in a module the preset-scoped fix does not touch.

How

  • Delegate path resolution to the canonical _assets resolvers (_locate_core_pack / _repo_root), exactly as presets/__init__.py does. These are anchored to the package root, so discovery survives future module moves.
  • Add tests/test_extensions.py::TestCoreCommandDiscovery covering the source, wheel, and fallback branches, plus a sentinel test proving discovery returns the real bundled set rather than the fallback.

Verification

  • New regression tests fail on the pre-fix code (the sentinel test fails with a clean assertion -- it returns the fallback sentinel instead of the bundled set) and pass after the fix.
  • tests/test_extensions.py: 328 passed, 3 skipped.
  • Broader relevant suites (imports, extensions, presets, integration registry/manifest): 887 passed. The only failures are 6 pre-existing Windows symlink-privilege errors in test_manifest.py that fail identically on the unmodified base (unrelated to this change).

Behavior change

None for end users today (the fallback already matches the real commands). This restores the intended dynamic discovery and removes the silent manual-maintenance dependency on the fallback list.


Authored with assistance from GitHub Copilot (model: Claude Opus 4.8), acting autonomously under the direction of @dhruv-15-03. Commits carry Assisted-by: trailers and the change is agent-generated; please review accordingly.

`_load_core_command_names()` resolved its command dirs with bespoke
Path(__file__) math that was correct only while the code lived at
src/specify_cli/extensions.py. The github#3014 refactor moved it into the
specify_cli/extensions/ package one directory deeper without updating the
parent counts, so both candidate dirs pointed at non-existent paths and
discovery always fell through to _FALLBACK_CORE_COMMAND_NAMES.

Delegate path resolution to the canonical _assets resolvers
(_locate_core_pack / _repo_root), mirroring presets/__init__.py. They are
anchored to the package root, so discovery is immune to future module
moves. Add regression tests that pin live discovery across the
wheel/source/fallback branches; they fail on the pre-fix code.

Fixes github#3274

Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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 fixes dynamic discovery of bundled core command templates used by the extensions system to prevent core-command shadowing. It corrects broken path resolution introduced when the extensions module moved into a package, by delegating lookup to the canonical _assets helpers and adding regression tests that ensure discovery doesn’t silently fall back to a hardcoded list.

Changes:

  • Update _load_core_command_names() to resolve command template directories via _locate_core_pack() (wheel) and _repo_root() (source) instead of Path(__file__) math.
  • Add regression tests covering: real discovery vs fallback, source-tree discovery, wheel core_pack preference, and fallback behavior/lockstep.

Reviewed changes

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

File Description
src/specify_cli/extensions/__init__.py Fixes core-command discovery by using _assets path resolvers (wheel + source) instead of brittle relative parent traversal.
tests/test_extensions.py Adds regression tests to ensure discovery reads real bundled templates and doesn’t silently rely on the fallback set.

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

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.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Low

@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 resolve conflicts

@dhruv-15-03

Copy link
Copy Markdown
Author

Closing this. The same fix already landed in #3287 (merged to main), resolving #3274 via the identical approach: delegating _load_core_command_names() to the canonical _locate_core_pack / _repo_root resolvers in _assets, with regression tests that point the resolvers at a temp tree to prove discovery reads from disk rather than the baked-in fallback.

With the core fix and the discovery/lockstep tests already on main, a rebase here would leave nothing meaningful behind, so there is no reason to keep it open. Two independent fixes converging on the _assets delegation is a good signal that it is the right anchor.

Thanks @mnriem for the review and @noor01mk for the fix. I will pick up something non-overlapping next.

Disclosure: prepared with GitHub Copilot (model: Claude Opus 4.8) assistance; posting on behalf of @dhruv-15-03.

@dhruv-15-03 dhruv-15-03 closed this Jul 2, 2026
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.

Bug: extensions core-command discovery is dead — off-by-one paths after the #3014 module move

3 participants