fix(extensions): restore core-command discovery broken by #3014 module move#3275
Open
v-dhruv wants to merge 1 commit into
Open
fix(extensions): restore core-command discovery broken by #3014 module move#3275v-dhruv wants to merge 1 commit into
v-dhruv wants to merge 1 commit into
Conversation
`_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>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the extensions subsystem where CORE_COMMAND_NAMES discovery became effectively dead after the extensions.py → extensions/__init__.py move (PR #3014). The loader now resolves bundled core command template paths using the canonical _assets helpers, restoring dynamic discovery and avoiding reliance on the hardcoded fallback set.
Changes:
- Update
_load_core_command_names()to resolve candidate command directories via_locate_core_pack()/_repo_root()instead ofPath(__file__)parent-counting. - Add regression tests covering source-tree discovery, wheel/core_pack discovery, the fallback branch, and a sentinel test to prove discovery isn’t silently returning the fallback.
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 | Fix core-command discovery by using canonical asset path resolvers instead of brittle relative __file__ math. |
| tests/test_extensions.py | Add regression coverage for core command discovery across source/wheel/fallback paths, including a sentinel to catch “dead discovery” masked by fallback equality. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #3274.Why
When this code moved from
src/specify_cli/extensions.pytosrc/specify_cli/extensions/__init__.py(#3014,826e193), the file went one directory deeper but the bespokePath(__file__)parent counts were not updated. Both candidate dirs now resolve to non-existent paths:specify_cli/extensions/core_pack/commands(real:specify_cli/core_pack/commands)src/templates/commands(real: repo-roottemplates/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 --convergewas 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
_assetsresolvers (_locate_core_pack/_repo_root), exactly aspresets/__init__.pydoes. These are anchored to the package root, so discovery survives future module moves.tests/test_extensions.py::TestCoreCommandDiscoverycovering the source, wheel, and fallback branches, plus a sentinel test proving discovery returns the real bundled set rather than the fallback.Verification
tests/test_extensions.py: 328 passed, 3 skipped.test_manifest.pythat 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 @v-dhruv. Commits carry
Assisted-by:trailers and the change is agent-generated; please review accordingly.