fix(extensions): restore core-command discovery broken by #3014 module move#3292
Open
dhruv-15-03 wants to merge 1 commit into
Open
fix(extensions): restore core-command discovery broken by #3014 module move#3292dhruv-15-03 wants to merge 1 commit into
dhruv-15-03 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
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 ofPath(__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.
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 #3294.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 @dhruv-15-03. Commits carry
Assisted-by:trailers and the change is agent-generated; please review accordingly.