Improve interpreter detection and add comprehensive test coverage

skills/developing-with-idfkit/scripts/discover.py

@@ -5,9 +5,11 @@
     python scripts/discover.py [--project-dir PATH]
 
 When --project-dir is given, the script resolves `.venv`, `../.venv`,
-`<git-root>/.venv`, `Pipfile`, `poetry.lock`, `pdm.lock`, and `uv.lock` relative
-to that path (so its checks land on the user's project rather than on the
-script's installed location).
+`<git-root>/.venv`, `Pipfile`, and the `poetry.lock`/`pdm.lock`/`uv.lock`
+lockfiles (each paired with a `pyproject.toml`) relative to that path (so its
+checks land on the user's project rather than on the script's installed
+location). An active conda `base` environment is ignored, since `CONDA_PREFIX`
+is exported merely by initializing conda.
 
 Exit codes:
     0 - success; prints the absolute path to the bundled SKILL.md on stdout.
@@ -99,21 +101,45 @@ def detect_interpreter(project_dir: Path) -> tuple[list[str], str] | None:
             return [str(py)], "venv-git-root"
 
     conda = os.environ.get("CONDA_PREFIX")
-    if conda:
+    # CONDA_PREFIX is exported pointing at `base` whenever conda is merely
+    # initialized, even with no project env active. Firing on that would advise
+    # `conda install ... idfkit` into base — the anti-pattern this branch avoids.
+    if (
+        conda
+        and Path(conda).name != "base"
+        and os.environ.get("CONDA_DEFAULT_ENV") != "base"
+    ):
         py = find_venv_python(Path(conda))
         if py:
             return [str(py)], "conda"
 
     if shutil.which("pipenv") and (project_dir / "Pipfile").is_file():
         return ["pipenv", "run", "python"], "pipenv"
 
-    if shutil.which("poetry") and (project_dir / "poetry.lock").is_file():
+    # poetry/pdm/uv only manage a project when a manifest is present. A bare
+    # lockfile makes them emit "could not find a pyproject.toml" on stderr
+    # instead of letting us give install advice, so require the manifest too.
+    has_pyproject = (project_dir / "pyproject.toml").is_file()
+
+    if (
+        shutil.which("poetry")
+        and (project_dir / "poetry.lock").is_file()
+        and has_pyproject
+    ):
         return ["poetry", "run", "python"], "poetry"
 
-    if shutil.which("pdm") and (project_dir / "pdm.lock").is_file():
+    if (
+        shutil.which("pdm")
+        and (project_dir / "pdm.lock").is_file()
+        and has_pyproject
+    ):
         return ["pdm", "run", "python"], "pdm"
 
-    if shutil.which("uv") and (project_dir / "uv.lock").is_file():
+    if (
+        shutil.which("uv")
+        and (project_dir / "uv.lock").is_file()
+        and has_pyproject
+    ):
         return ["uv", "run", "--quiet", "python"], "uv"
 
     for name in ("python3", "python"):

tests/discovery/test_discover.py

@@ -10,7 +10,9 @@
 
 from __future__ import annotations
 
+import contextlib
 import importlib.util
+import io
 import os
 import subprocess
 import sys
@@ -78,10 +80,127 @@ def fake_find(root: Path):
             self.assertEqual(tag, "venv-local")
             self.assertEqual(cmd, [str(Path("/proj/.venv/bin/python"))])
 
+    def test_parent_venv(self):
+        project = Path("/proj/sub")
+
+        def fake_find(root: Path):
+            return Path("/proj/.venv/bin/python") if root == project.parent / ".venv" else None
+
+        with mock.patch.dict(os.environ, {}, clear=True), \
+             mock.patch.object(discover, "find_venv_python", side_effect=fake_find):
+            cmd, tag = discover.detect_interpreter(project)
+            self.assertEqual(tag, "venv-parent")
+            self.assertEqual(cmd, [str(Path("/proj/.venv/bin/python"))])
+
+    def test_git_root_venv(self):
+        project = Path("/repo/a/b")
+        git_root = Path("/repo")
+
+        def fake_find(root: Path):
+            return Path("/repo/.venv/bin/python") if root == git_root / ".venv" else None
+
+        with mock.patch.dict(os.environ, {}, clear=True), \
+             mock.patch.object(discover, "find_venv_python", side_effect=fake_find), \
+             mock.patch.object(discover, "find_git_root", return_value=git_root):
+            cmd, tag = discover.detect_interpreter(project)
+            self.assertEqual(tag, "venv-git-root")
+            self.assertEqual(cmd, [str(Path("/repo/.venv/bin/python"))])
+
+    def test_conda_env(self):
+        conda_prefix = "/opt/conda/envs/myenv"
+
+        def fake_find(root: Path):
+            return Path(conda_prefix) / "bin" / "python" if root == Path(conda_prefix) else None
+
+        with mock.patch.dict(
+            os.environ,
+            {"CONDA_PREFIX": conda_prefix, "CONDA_DEFAULT_ENV": "myenv"},
+            clear=True,
+        ), \
+             mock.patch.object(discover, "find_venv_python", side_effect=fake_find), \
+             mock.patch.object(discover, "find_git_root", return_value=None):
+            cmd, tag = discover.detect_interpreter(Path("/proj"))
+            self.assertEqual(tag, "conda")
+            self.assertEqual(cmd, [str(Path(conda_prefix) / "bin" / "python")])
+
+    def test_conda_base_skipped_by_default_env(self):
+        # CONDA_DEFAULT_ENV == "base": skip even though a base python exists,
+        # falling through to the system interpreter.
+        conda_prefix = "/opt/miniconda3"
+
+        def fake_find(root: Path):
+            return Path(conda_prefix) / "bin" / "python" if root == Path(conda_prefix) else None
+
+        with mock.patch.dict(
+            os.environ,
+            {"CONDA_PREFIX": conda_prefix, "CONDA_DEFAULT_ENV": "base"},
+            clear=True,
+        ), \
+             mock.patch.object(discover, "find_venv_python", side_effect=fake_find), \
+             mock.patch.object(discover, "find_git_root", return_value=None), \
+             mock.patch.object(discover.shutil, "which",
+                               side_effect=lambda n: n == "python3"):
+            cmd, tag = discover.detect_interpreter(Path("/proj"))
+            self.assertEqual(tag, "system")
+
+    def test_conda_base_skipped_by_prefix_basename(self):
+        # CONDA_PREFIX basename == "base" (CONDA_DEFAULT_ENV unset): also skip.
+        conda_prefix = "/opt/conda/envs/base"
+
+        def fake_find(root: Path):
+            return Path(conda_prefix) / "bin" / "python" if root == Path(conda_prefix) else None
+
+        with mock.patch.dict(os.environ, {"CONDA_PREFIX": conda_prefix}, clear=True), \
+             mock.patch.object(discover, "find_venv_python", side_effect=fake_find), \
+             mock.patch.object(discover, "find_git_root", return_value=None), \
+             mock.patch.object(discover.shutil, "which",
+                               side_effect=lambda n: n == "python3"):
+            cmd, tag = discover.detect_interpreter(Path("/proj"))
+            self.assertEqual(tag, "system")
+
+    def test_pipenv(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            project = Path(tmp)
+            _touch(project / "Pipfile")
+            with mock.patch.dict(os.environ, {}, clear=True), \
+                 mock.patch.object(discover, "find_venv_python", return_value=None), \
+                 mock.patch.object(discover, "find_git_root", return_value=None), \
+                 mock.patch.object(discover.shutil, "which", side_effect=lambda n: n == "pipenv"):
+                cmd, tag = discover.detect_interpreter(project)
+                self.assertEqual(tag, "pipenv")
+                self.assertEqual(cmd, ["pipenv", "run", "python"])
+
+    def test_poetry_with_manifest(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            project = Path(tmp)
+            _touch(project / "poetry.lock")
+            _touch(project / "pyproject.toml")
+            with mock.patch.dict(os.environ, {}, clear=True), \
+                 mock.patch.object(discover, "find_venv_python", return_value=None), \
+                 mock.patch.object(discover, "find_git_root", return_value=None), \
+                 mock.patch.object(discover.shutil, "which", side_effect=lambda n: n == "poetry"):
+                cmd, tag = discover.detect_interpreter(project)
+                self.assertEqual(tag, "poetry")
+                self.assertEqual(cmd, ["poetry", "run", "python"])
+
+    def test_pdm_with_manifest(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            project = Path(tmp)
+            _touch(project / "pdm.lock")
+            _touch(project / "pyproject.toml")
+            with mock.patch.dict(os.environ, {}, clear=True), \
+                 mock.patch.object(discover, "find_venv_python", return_value=None), \
+                 mock.patch.object(discover, "find_git_root", return_value=None), \
+                 mock.patch.object(discover.shutil, "which", side_effect=lambda n: n == "pdm"):
+                cmd, tag = discover.detect_interpreter(project)
+                self.assertEqual(tag, "pdm")
+                self.assertEqual(cmd, ["pdm", "run", "python"])
+
     def test_uv_lockfile(self):
         with tempfile.TemporaryDirectory() as tmp:
             project = Path(tmp)
             _touch(project / "uv.lock")
+            _touch(project / "pyproject.toml")
             with mock.patch.dict(os.environ, {}, clear=True), \
                  mock.patch.object(discover, "find_venv_python", return_value=None), \
                  mock.patch.object(discover, "find_git_root", return_value=None), \
@@ -90,6 +209,20 @@ def test_uv_lockfile(self):
                 self.assertEqual(tag, "uv")
                 self.assertEqual(cmd, ["uv", "run", "--quiet", "python"])
 
+    def test_lockfile_without_manifest_falls_through(self):
+        # uv.lock present but no pyproject.toml: the uv branch must not fire;
+        # detection falls through to the system interpreter.
+        with tempfile.TemporaryDirectory() as tmp:
+            project = Path(tmp)
+            _touch(project / "uv.lock")  # no pyproject.toml alongside it
+            with mock.patch.dict(os.environ, {}, clear=True), \
+                 mock.patch.object(discover, "find_venv_python", return_value=None), \
+                 mock.patch.object(discover, "find_git_root", return_value=None), \
+                 mock.patch.object(discover.shutil, "which",
+                                   side_effect=lambda n: n in {"uv", "python3"}):
+                cmd, tag = discover.detect_interpreter(project)
+                self.assertEqual(tag, "system")
+
     def test_system_fallback(self):
         with tempfile.TemporaryDirectory() as tmp:
             project = Path(tmp)  # no lockfiles present
@@ -147,5 +280,62 @@ def test_bad_project_dir_exits_5(self):
         self.assertIn("not a directory", result.stderr)
 
 
+class MainExitCodeTests(unittest.TestCase):
+    """Drive main() with a faked interpreter + subprocess to exercise the
+    import-result branches (exit 0/1/2/4) deterministically and cross-platform.
+    """
+
+    def _run_main(self, *, returncode, stdout, stderr=""):
+        fake_proc = subprocess.CompletedProcess(
+            args=[], returncode=returncode, stdout=stdout, stderr=stderr,
+        )
+        out, err = io.StringIO(), io.StringIO()
+        with mock.patch.object(sys, "argv", ["discover.py"]), \
+             mock.patch.object(discover, "detect_interpreter",
+                               return_value=(["python3"], "system")), \
+             mock.patch.object(discover.subprocess, "run", return_value=fake_proc), \
+             contextlib.redirect_stdout(out), \
+             contextlib.redirect_stderr(err):
+            code = discover.main()
+        return code, out.getvalue(), err.getvalue()
+
+    def test_exit_0_prints_skill_path(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            pkg = Path(tmp)
+            skill = pkg / ".agents" / "skills" / "developing-with-idfkit" / "SKILL.md"
+            _touch(skill)
+            code, out, _ = self._run_main(returncode=0, stdout=f"{pkg}\n")
+            self.assertEqual(code, 0)
+            self.assertEqual(out.strip(), str(skill.resolve()))
+
+    def test_exit_4_when_layout_changed(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            pkg = Path(tmp)
+            # .agents/skills exists with some entry, but the documented
+            # developing-with-idfkit/SKILL.md is absent.
+            _touch(pkg / ".agents" / "skills" / "some-other-skill" / "SKILL.md")
+            code, _, err = self._run_main(returncode=0, stdout=f"{pkg}\n")
+            self.assertEqual(code, 4)
+            self.assertIn("some-other-skill", err)
+            self.assertIn(discover.DOCS_FALLBACK, err)
+
+    def test_exit_2_when_idfkit_too_old(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            pkg = Path(tmp)  # installed idfkit, but no .agents/skills/ at all
+            code, _, err = self._run_main(returncode=0, stdout=f"{pkg}\n")
+            self.assertEqual(code, 2)
+            self.assertIn("predates bundled skills", err)
+            self.assertIn(discover.DOCS_FALLBACK, err)
+
+    def test_exit_1_when_not_installed(self):
+        code, _, err = self._run_main(
+            returncode=1,
+            stdout="",
+            stderr="ModuleNotFoundError: No module named 'idfkit'",
+        )
+        self.assertEqual(code, 1)
+        self.assertIn("not installed", err)
+
+
 if __name__ == "__main__":
     unittest.main()