From 323bf4d0fbf35bbbb52e7c89aeb55b83be3592fc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 27 May 2026 12:18:22 +0000 Subject: [PATCH] Harden discover.py interpreter detection and cover exit codes 2/4 Skip conda when the active env is `base` (CONDA_PREFIX is exported merely by initializing conda), so install advice no longer targets the base env. Gate the poetry/pdm/uv branches on a pyproject.toml so a bare lockfile no longer leaks the tool's "no pyproject.toml" stderr instead of install advice. Add tests for the previously uncovered detection branches (conda, venv-parent, venv-git-root, pipenv, poetry, pdm) and drive main() through exit codes 0/1/2/4. Closes #5 https://claude.ai/code/session_011vxyvBpp9229dcqBMyTGNT --- .../scripts/discover.py | 40 +++- tests/discovery/test_discover.py | 190 ++++++++++++++++++ 2 files changed, 223 insertions(+), 7 deletions(-) diff --git a/skills/developing-with-idfkit/scripts/discover.py b/skills/developing-with-idfkit/scripts/discover.py index 8d43429..d2ae8f2 100755 --- a/skills/developing-with-idfkit/scripts/discover.py +++ b/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`, -`/.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). +`/.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,7 +101,14 @@ 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" @@ -107,13 +116,30 @@ def detect_interpreter(project_dir: Path) -> tuple[list[str], str] | None: 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"): diff --git a/tests/discovery/test_discover.py b/tests/discovery/test_discover.py index acc6e45..0c27b76 100644 --- a/tests/discovery/test_discover.py +++ b/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()