mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
`_load_core_command_names()` computed its candidate command dirs with bespoke `Path(__file__)` arithmetic. The #3014 move of this module from `specify_cli/extensions.py` to `specify_cli/extensions/__init__.py` pushed the file one directory deeper but left the `.parent` counts unchanged, so both candidates resolved 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) Neither exists, so every call silently fell through to `_FALLBACK_CORE_COMMAND_NAMES`. Discovery is latent-dead: the fallback happens to equal the real stems today, but the shadowing guard (#1994) that depends on it now relies on someone hand-editing the fallback on every core-command add/remove (as already happened for `converge`, #3001). Delegate path resolution to the canonical `_locate_core_pack` / `_repo_root` resolvers in `_assets` — the same ones the presets and bundle loaders use. They are anchored to the package root, so discovery survives future module moves. Add regression tests that point the resolvers at a temp tree with *different* command names, proving discovery reads from disk rather than returning the fallback (they fail on the pre-fix code). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -26,6 +26,7 @@ import yaml
|
||||
from packaging import version as pkg_version
|
||||
from packaging.specifiers import InvalidSpecifier, SpecifierSet
|
||||
|
||||
from .._assets import _locate_core_pack, _repo_root
|
||||
from .._init_options import is_ai_skills_enabled
|
||||
from .._invocation_style import is_dollar_skills_agent, is_slash_skills_agent
|
||||
from .._utils import dump_frontmatter, relative_extension_path_violation, version_satisfies
|
||||
@@ -62,14 +63,28 @@ def _load_core_command_names() -> frozenset[str]:
|
||||
Prefer the wheel-time ``core_pack`` bundle when present, and fall back to
|
||||
the source checkout when running from the repository. If neither is
|
||||
available, use the baked-in fallback set so validation still works.
|
||||
|
||||
Path resolution is delegated to the canonical ``_assets`` resolvers
|
||||
(``_locate_core_pack`` / ``_repo_root``) — the same ones the presets and
|
||||
bundle loaders use — rather than bespoke ``Path(__file__)`` arithmetic.
|
||||
Hand-counted ``.parent`` chains silently broke discovery once already: the
|
||||
#3014 move of this module from ``specify_cli/extensions.py`` to
|
||||
``specify_cli/extensions/__init__.py`` pushed the file one directory deeper
|
||||
without updating the counts, so both candidates resolved to non-existent
|
||||
paths and every call fell through to the fallback (#3274). The shared
|
||||
resolvers are anchored to the package root, so discovery survives future
|
||||
module moves.
|
||||
"""
|
||||
core_pack = _locate_core_pack()
|
||||
candidate_dirs = [
|
||||
Path(__file__).parent / "core_pack" / "commands",
|
||||
Path(__file__).resolve().parent.parent.parent / "templates" / "commands",
|
||||
# Wheel install: force-include maps templates/commands → core_pack/commands.
|
||||
core_pack / "commands" if core_pack is not None else None,
|
||||
# Source checkout / editable install: repo-root templates/commands.
|
||||
_repo_root() / "templates" / "commands",
|
||||
]
|
||||
|
||||
for commands_dir in candidate_dirs:
|
||||
if not commands_dir.is_dir():
|
||||
if commands_dir is None or not commands_dir.is_dir():
|
||||
continue
|
||||
|
||||
command_names = {
|
||||
|
||||
@@ -233,6 +233,73 @@ class TestExtensionManifest:
|
||||
|
||||
assert CORE_COMMAND_NAMES == expected
|
||||
|
||||
def test_load_core_command_names_discovers_from_source_checkout(self, monkeypatch):
|
||||
"""Discovery must actually read the repo-root templates, not silently
|
||||
fall back (#3274).
|
||||
|
||||
The fallback set happens to equal the real command stems today, so an
|
||||
equality check against the live tree cannot tell a working loader apart
|
||||
from a dead one. Point ``_repo_root`` at a temp tree with *different*
|
||||
command names: the old off-by-one path math read nothing and returned
|
||||
the baked-in fallback; the fixed loader returns the temp stems.
|
||||
"""
|
||||
from specify_cli.extensions import (
|
||||
_load_core_command_names,
|
||||
_FALLBACK_CORE_COMMAND_NAMES,
|
||||
)
|
||||
import specify_cli.extensions as ext
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
commands = Path(tmp) / "templates" / "commands"
|
||||
commands.mkdir(parents=True)
|
||||
(commands / "widget.md").write_text("# widget", encoding="utf-8")
|
||||
(commands / "gadget.md").write_text("# gadget", encoding="utf-8")
|
||||
(commands / "notacommand.txt").write_text("skip me", encoding="utf-8")
|
||||
|
||||
# No wheel bundle in this scenario; force the source-checkout path.
|
||||
monkeypatch.setattr(ext, "_locate_core_pack", lambda: None)
|
||||
monkeypatch.setattr(ext, "_repo_root", lambda: Path(tmp))
|
||||
|
||||
result = _load_core_command_names()
|
||||
|
||||
assert result == {"widget", "gadget"}
|
||||
assert result != _FALLBACK_CORE_COMMAND_NAMES
|
||||
|
||||
def test_load_core_command_names_prefers_wheel_core_pack(self, monkeypatch):
|
||||
"""When a wheel ``core_pack`` bundle exists, discovery reads
|
||||
``core_pack/commands`` (the force-include target) ahead of the source
|
||||
tree (#3274)."""
|
||||
from specify_cli.extensions import _load_core_command_names
|
||||
import specify_cli.extensions as ext
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
core_pack = Path(tmp) / "core_pack"
|
||||
(core_pack / "commands").mkdir(parents=True)
|
||||
(core_pack / "commands" / "sprocket.md").write_text("# sprocket", encoding="utf-8")
|
||||
|
||||
monkeypatch.setattr(ext, "_locate_core_pack", lambda: core_pack)
|
||||
# Source fallback should be ignored while the bundle resolves.
|
||||
monkeypatch.setattr(ext, "_repo_root", lambda: Path(tmp) / "nonexistent")
|
||||
|
||||
result = _load_core_command_names()
|
||||
|
||||
assert result == {"sprocket"}
|
||||
|
||||
def test_load_core_command_names_falls_back_when_nothing_found(self, monkeypatch):
|
||||
"""With neither a bundle nor a source tree, discovery returns the
|
||||
baked-in fallback so validation still works (#3274)."""
|
||||
from specify_cli.extensions import (
|
||||
_load_core_command_names,
|
||||
_FALLBACK_CORE_COMMAND_NAMES,
|
||||
)
|
||||
import specify_cli.extensions as ext
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
monkeypatch.setattr(ext, "_locate_core_pack", lambda: None)
|
||||
monkeypatch.setattr(ext, "_repo_root", lambda: Path(tmp) / "nonexistent")
|
||||
|
||||
assert _load_core_command_names() == _FALLBACK_CORE_COMMAND_NAMES
|
||||
|
||||
def test_missing_required_field(self, temp_dir):
|
||||
"""Test manifest missing required field."""
|
||||
import yaml
|
||||
|
||||
Reference in New Issue
Block a user