From e3bb151a7504e0bcb34dc93e1a1c1aa2ba2f2a92 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 1 Jul 2026 16:58:50 +0500 Subject: [PATCH] fix(extensions): resolve core-command dirs via _assets helpers (#3274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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) --- src/specify_cli/extensions/__init__.py | 21 ++++++-- tests/test_extensions.py | 67 ++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 3dd46ee6d2..b9f81a3955 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -26,6 +26,7 @@ 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 @@ -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 = { diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 6b181a1204..16ffcd14d3 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -227,6 +227,73 @@ def test_core_command_names_match_bundled_templates(self): 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