test(discover): isolate fallback tests from RENDERDOC_PYTHON_PATH env leak

openspec/changes/2026-06-09-discover-test-isolation/proposal.md

@@ -0,0 +1,106 @@
+# Proposal: discover test isolation — RENDERDOC_PYTHON_PATH env leak
+
+## Root cause
+
+`test_import_failed_without_module_leaves_no_diagnostic`
+(`tests/unit/test_discover.py:274`) fails when run under `pixi run` because
+`pixi.toml` sets `RENDERDOC_PYTHON_PATH = ".local/renderdoc"` in the activation
+environment, and the test does not neutralise that variable.
+
+### Exact mechanism (file:line)
+
+`find_renderdoc()` in `src/rdc/discover.py:150` reads `os.environ.get("RENDERDOC_PYTHON_PATH")`.
+When the variable is set to `.local/renderdoc` (a symlink that resolves to
+`.local/renderdoc` in the main worktree, which contains `renderdoc.so`),
+`os.path.abspath(env_path)` produces a valid directory that is prepended to the
+`candidates` list.
+
+The test mocks `_probe_candidate` with a lambda that ignores its `path` argument
+and unconditionally returns `ProbeOutcome(IMPORT_FAILED, str(empty_dir))`.  When
+`find_renderdoc()` iterates over `candidates` it calls this mock with the real
+`.local/renderdoc` path, receives `IMPORT_FAILED` back, then evaluates:
+
+```python
+# discover.py:187
+elif outcome.result == ProbeResult.IMPORT_FAILED and _has_renderdoc_module(path):
+    _diagnostic = outcome
+```
+
+`_has_renderdoc_module(path)` is called with the **real** `.local/renderdoc` path
+(not `empty_dir`).  That directory contains `renderdoc.so`, so the function
+returns `True`, and `_diagnostic` is set — making the test's final assertion
+`assert _get_diagnostic() is None` fail.
+
+The `empty_dir` fixture has no `.so` / `.py` / `.pyd` files, so the guard at line
+187 was intended to be False.  The mismatch arises because the mock returns an
+outcome whose `candidate_path` points to `empty_dir`, while `_has_renderdoc_module`
+is evaluated against the loop variable `path`, which is the env-path candidate.
+
+### Why it is env-dependent
+
+| Condition | `RENDERDOC_PYTHON_PATH` in env | `.local/renderdoc` exists | Result |
+|-----------|-------------------------------|--------------------------|--------|
+| Clean checkout, no `pixi run sync` | not set or empty | no | PASS |
+| `pixi run` (activation sets var) | `.local/renderdoc` | no | PASS (dir missing, not added) |
+| `pixi run` after `pixi run sync` | `.local/renderdoc` | yes, has `renderdoc.so` | **FAIL** |
+| `pixi run` after `pixi run sync`, even in isolation | `.local/renderdoc` | yes | **FAIL** |
+
+Agent 1 ran after `pixi run sync` (real renderdoc built) — fails.  Agent 2 ran
+without `pixi run sync` (`.local/renderdoc` absent or not yet populated) — passes.
+
+## Fix design
+
+The test's declared precondition is "import failed without a module present".  It
+must hold regardless of the ambient `RENDERDOC_PYTHON_PATH`.  The correct fix is
+to isolate the environment inside the test: use
+`monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` so that no env
+candidate is injected into `candidates`.
+
+This is a **test fix, not a product bug**.  `discover.py` correctly reads the env
+variable and correctly applies `_has_renderdoc_module`; the real `.local/renderdoc`
+directory does contain a module file, so setting `_diagnostic` there is the right
+behaviour.  The test simply failed to declare an assumption about the environment.
+
+The same isolation pattern should also be applied to the three sibling tests in
+`TestFindRenderdocFallback` that mock `renderdoc_search_paths` and `shutil.which`
+but also leave `RENDERDOC_PYTHON_PATH` unmocked.  They currently pass only by
+accident, each for a different reason:
+
+- `test_skips_crash_prone_candidate` (line 167) uses a two-outcome iterator;
+  with the env candidate present, the `CRASH_PRONE` outcome lands on the env
+  path and `SUCCESS` lands on `crash_dir` — the assertion still holds because
+  `_try_import_from` is mocked to return the module for any directory, but the
+  test no longer exercises the scenario it claims, and a third candidate would
+  raise `StopIteration`.
+- `test_diagnostic_set_after_crash_prone` (line 207): the env candidate just
+  adds one more `CRASH_PRONE` probe whose outcome carries `crash_dir`, so the
+  assertions hold.
+- `test_diagnostic_set_after_import_failed` (line 237): the env candidate
+  probes `IMPORT_FAILED` and `_has_renderdoc_module(env path)` is `True`, so
+  `_diagnostic` is set from the env candidate too — the assertions hold only
+  because the mocked outcome's fields point at `bad_dir` regardless.
+
+All three carry a latent dependency on the ambient env.  Clearing the variable
+in all four tests makes the suite robust.
+
+### Minimal change
+
+In `tests/unit/test_discover.py`, add one line to each of the four tests in
+`TestFindRenderdocFallback`:
+
+```python
+monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)
+```
+
+Place it alongside the other `monkeypatch.setattr` calls at the top of each test
+body.
+
+No changes to `src/rdc/discover.py`.
+
+## Risks
+
+| Risk | Mitigation |
+|------|------------|
+| Other env vars also leak into `find_renderdoc()` | `RENDERDOC_PYTHON_PATH` is the only env var read directly (discover.py:150). `PATH` reaches the candidate list indirectly via `shutil.which("renderdoccmd")` (discover.py:159-161) and `PYTHONPATH`/`sys.path` reach `_try_import` — both are already mocked in all four tests, so `delenv` plus the existing mocks give full isolation. |
+| Removing env var hides a real test scenario | The env-path scenario is covered by `test_diagnostic_set_after_crash_prone` and `test_diagnostic_set_after_import_failed`; those tests can explicitly set `RENDERDOC_PYTHON_PATH` to a tmp dir if desired. |
+| Sibling tests in `TestTryImportFrom` / `TestProbeCandidate` | Those tests do not call `find_renderdoc()` and are unaffected. |

openspec/changes/2026-06-09-discover-test-isolation/tasks.md

@@ -0,0 +1,27 @@
+# Tasks: discover-test-isolation
+
+## Phase A: fix the failing test
+
+- [x] In `tests/unit/test_discover.py`,
+  `TestFindRenderdocFallback.test_import_failed_without_module_leaves_no_diagnostic` (line 274):
+  add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` alongside the other
+  `monkeypatch.setattr` calls.
+
+## Phase B: harden sibling tests
+
+- [x] `test_skips_crash_prone_candidate` (line 167): add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)`.
+- [x] `test_diagnostic_set_after_crash_prone` (line 207): same.
+- [x] `test_diagnostic_set_after_import_failed` (line 237): same.
+
+## Phase C: add regression guard (TC-2)
+
+- [x] Add `test_import_failed_with_env_module_sets_diagnostic` to
+  `TestFindRenderdocFallback`, implementing test-plan TC-2 exactly (tmp-dir
+  `mkdir()` + `renderdoc.so` stub file, `setenv`, mocks, assert no module +
+  diagnostic `IMPORT_FAILED` with `candidate_path == str(real_dir)`).
+
+## Phase D: verification
+
+- [x] Run `pixi run uv run pytest tests/unit/test_discover.py -v` — all 17+ tests pass.
+- [x] Run `pixi run test` — full unit suite passes, coverage ≥ 80%.
+- [x] Confirm TC-1 passes with `RENDERDOC_PYTHON_PATH` set in the outer env (i.e. within `pixi run`).

openspec/changes/2026-06-09-discover-test-isolation/test-plan.md

@@ -0,0 +1,64 @@
+# Test plan: discover test isolation
+
+## Regression guard (primary)
+
+The corrected test must pass in BOTH environments:
+
+| Environment | Expected result |
+|-------------|----------------|
+| `pixi run test` with `pixi run sync` done (`.local/renderdoc` populated, `RENDERDOC_PYTHON_PATH=.local/renderdoc`) | PASS |
+| Clean env, `RENDERDOC_PYTHON_PATH` unset | PASS |
+| `RENDERDOC_PYTHON_PATH` set to a directory that contains `renderdoc.so` | PASS |
+
+To verify the "real renderdoc present" case explicitly, run:
+
+```
+pixi run uv run pytest tests/unit/test_discover.py::TestFindRenderdocFallback::test_import_failed_without_module_leaves_no_diagnostic -v
+```
+
+This already exercises the failure condition (pixi activation sets `RENDERDOC_PYTHON_PATH`).
+After the fix is applied this must exit 0 in this worktree.
+
+## Test cases
+
+### TC-1 — originally failing: no-module dir, env var populated
+
+- Setup: `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` added.
+- Input: `empty_dir` (no `.so` / `.py` / `.pyd`), `_probe_candidate` returns
+  `IMPORT_FAILED`, `renderdoc_search_paths` returns `[str(empty_dir)]`,
+  `shutil.which` returns `None`.
+- Expected: `find_renderdoc()` returns `None`; `_get_diagnostic()` returns `None`.
+- Must pass with `RENDERDOC_PYTHON_PATH=.local/renderdoc` in the outer env.
+
+### TC-2 — regression guard: env-path candidate with real module produces diagnostic
+
+- New test (or inline parametrize).
+- Setup: `real_dir = tmp_path / "real-rdoc"; real_dir.mkdir()`; write a stub
+  `(real_dir / "renderdoc.so").write_bytes(b"fake")`. A plain file named
+  `renderdoc.so` is sufficient: `_has_renderdoc_module` (discover.py:59-66)
+  only checks `renderdoc.py` is_file / glob `renderdoc*.so` / glob
+  `renderdoc*.pyd`; it never loads the file. `monkeypatch.setenv("RENDERDOC_PYTHON_PATH", str(real_dir))`.
+  Mock `_try_import -> None`,
+  `_probe_candidate -> ProbeOutcome(IMPORT_FAILED, str(real_dir))`,
+  `_try_import_from -> None`, `renderdoc_search_paths -> []`, `shutil.which -> None`.
+- Expected: `find_renderdoc()` returns `None`; `_get_diagnostic()` is **not** `None`,
+  has `result == IMPORT_FAILED` and `candidate_path == str(real_dir)`.
+- Deterministic in both ambient env states (the test sets the variable itself),
+  so it runs green inside and outside `pixi run`.
+- This documents and protects the product behaviour (env-path module present → diagnostic set).
+
+### TC-3 through TC-5 — latent env leak in sibling tests
+
+Each of `test_skips_crash_prone_candidate`, `test_diagnostic_set_after_crash_prone`,
+and `test_diagnostic_set_after_import_failed` must include:
+
+```python
+monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)
+```
+
+These tests already pass but carry a hidden dependency.  Adding the `delenv` call
+makes them unconditionally safe.  Confirm they still pass after the addition.
+
+## Full suite check
+
+Run `pixi run test` (all unit tests, coverage ≥ 80%); all must pass.

tests/unit/test_discover.py

@@ -194,6 +194,7 @@ def mock_renderdoc_search_paths():
         def mock_which(cmd: str):
             return None
 
+        monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)
         monkeypatch.setattr("rdc.discover._try_import", lambda: None)
         monkeypatch.setattr("rdc.discover._probe_candidate", mock_probe)
         monkeypatch.setattr("rdc.discover._try_import_from", lambda d: real_fake_mod)
@@ -217,6 +218,7 @@ def mock_renderdoc_search_paths():
         def mock_which(cmd: str):
             return None
 
+        monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)
         monkeypatch.setattr("rdc.discover._try_import", lambda: None)
         monkeypatch.setattr(
             "rdc.discover._probe_candidate",
@@ -254,6 +256,7 @@ def mock_renderdoc_search_paths():
         def mock_which(cmd: str):
             return None
 
+        monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)
         monkeypatch.setattr("rdc.discover._try_import", lambda: None)
         monkeypatch.setattr(
             "rdc.discover._probe_candidate",
@@ -280,6 +283,7 @@ def test_import_failed_without_module_leaves_no_diagnostic(
         empty_dir = tmp_path / "no-module"
         empty_dir.mkdir()
 
+        monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)
         monkeypatch.setattr("rdc.discover._try_import", lambda: None)
         monkeypatch.setattr(
             "rdc.discover._probe_candidate",
@@ -292,6 +296,32 @@ def test_import_failed_without_module_leaves_no_diagnostic(
         assert find_renderdoc() is None
         assert _get_diagnostic() is None
 
+    def test_import_failed_with_env_module_sets_diagnostic(
+        self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path
+    ) -> None:
+        """An env-path candidate holding a module artifact that fails to import
+        records a diagnostic, even with all other candidate sources empty.
+        """
+        real_dir = tmp_path / "real-rdoc"
+        real_dir.mkdir()
+        (real_dir / "renderdoc.so").write_bytes(b"fake")
+
+        monkeypatch.setenv("RENDERDOC_PYTHON_PATH", str(real_dir))
+        monkeypatch.setattr("rdc.discover._try_import", lambda: None)
+        monkeypatch.setattr(
+            "rdc.discover._probe_candidate",
+            lambda path, timeout=5.0: ProbeOutcome(ProbeResult.IMPORT_FAILED, str(real_dir)),
+        )
+        monkeypatch.setattr("rdc.discover._try_import_from", lambda d: None)
+        monkeypatch.setattr("rdc._platform.renderdoc_search_paths", lambda: [])
+        monkeypatch.setattr("rdc.discover.shutil.which", lambda cmd: None)
+
+        assert find_renderdoc() is None
+        diag = _get_diagnostic()
+        assert diag is not None
+        assert diag.result == ProbeResult.IMPORT_FAILED
+        assert diag.candidate_path == str(real_dir)
+
 
 class TestArmStudioDir:
     """_is_arm_studio_dir detects ARM PS directory layout."""