Fix #2065: decouple test/test-all from venv prerequisite

[jwbron]

Summary

• The sandbox container pre-installs pytest/ruff/mypy globally (sandbox/Dockerfile lines 156-178) but does not ship uv. The Makefile comment at lines 12-13 already documents this design ("CI uses venv (via uv sync); the sandbox has tools installed globally"), but test: and test-all: were declared with venv as a hard prerequisite — and venv: exits with ERROR: uv is not installed. when uv is missing.
• Result: the BRC tester inside the sandbox could never invoke make test, fell back to ad-hoc pytest invocations that missed half of pyproject.toml's testpaths, and several failing tests slipped through to CI on PR Improve generalized reviewer with subagent fan-out and lens reviewers #2061 (see run 24937921821).
• Add a sync-venv-if-uv target that delegates to venv only when uv is on PATH; depend on it from test: and test-all:. Dev machines and CI continue to run uv sync --extra dev exactly as before; the sandbox now uses the existing PYTEST/PYTHON system-PATH fallbacks the Makefile already wires up.

Closes #2065.

Test plan

• make test and make test-all still trigger uv sync on a host with uv installed.
• env -i PATH=/bin:/usr/bin make sync-venv-if-uv is a clean no-op when uv is absent.
• Confirm in-sandbox make test runs the full changeset-aware selection on the next BRC run.

🤖 Generated with Claude Code

[egg-reviewer]

Review: PR #2072 — decouple test/test-all from venv prerequisite

Focused, well-targeted Makefile fix. The diagnosis matches the code: venv: (Makefile:105-114) hard-fails with ERROR: uv is not installed. when uv is absent, and test: / test-all: previously listed it as a strict prerequisite. The sandbox image (sandbox/Dockerfile:141-178) installs pytest/ruff/mypy via pip3 but does not install uv, so the prereq couldn't be satisfied there. The new sync-venv-if-uv target (Makefile:118-124) preserves uv sync behaviour on dev machines and CI (where uv is present) while degrading to a clean no-op in the sandbox, where the existing PYTEST/PYTHON system-PATH fallbacks (Makefile:14-19) take over.

No blocking issues. CI is green (Unit Tests + lint pass on this branch).

Non-blocking observations

1. Other venv-prereq targets are unchanged. smoketest-long-poll (Makefile:356), test-integration (Makefile:381), test-e2e (Makefile:385), and test-security (Makefile:389) still hard-depend on venv. They require Docker/API keys and aren't expected to run inside the sandbox, so this is fine for the scope of #2065 — flagging only because if the BRC tester ever invokes smoketest-long-poll, it'll hit the same ERROR: uv is not installed. wall this PR was written to remove.

2. Test plan item still unchecked. The PR body marks "Confirm in-sandbox make test runs the full changeset-aware selection on the next BRC run" as unverified. Worth observing on the next sandbox run before considering #2065 fully closed — local dev testing doesn't exercise the failure mode the PR fixes.

3. Failure-mode UX trade-off. When uv is missing, the previous venv: recipe produced an actionable "ERROR: uv is not installed." with install instructions. With this change, a misconfigured environment that lacks both uv and pytest on PATH will silently no-op through sync-venv-if-uv, then fail later with a less specific pytest: command not found from line 311. The comment block on sync-venv-if-uv documents the intent clearly enough that I don't think this needs a guard, but worth being aware of.

4. $(MAKE) invocation pattern. Line 124 invokes $(MAKE) venv from inside an @if shell conditional, mirroring the existing deps: pattern at line 99. Neither uses the + prefix, so neither propagates the GNU Make jobserver — fine here since uv sync is a single process, just noting it's consistent with the existing convention rather than something to change.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the thorough review. Acknowledging the four non-blocking observations:

1. Other venv-prereq targets (smoketest-long-poll, test-integration, test-e2e, test-security) — Agreed this is out of scope for Sandbox container missing uv: BRC tester can't run make test, falls back to incomplete pytest #2065. These targets need Docker/API keys and aren't run inside the sandbox today. If a future BRC role needs smoketest-long-poll from the sandbox, we can extend sync-venv-if-uv to those targets in a follow-up.

2. Unchecked test plan item — Agreed. Will be confirmed on the next in-sandbox BRC run; not verifiable from a dev host.

3. Failure-mode UX trade-off — Agreed with the assessment that the comment block on sync-venv-if-uv documents intent clearly. The downstream pytest: command not found failure is loud enough, and adding a guard here would re-introduce the exact failure mode this PR removes for the sandbox case.

4. $(MAKE) jobserver propagation — Noted, no change needed since this is consistent with the existing deps: pattern and uv sync is single-process.

No code changes required. Leaving as-is.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

1 previous review(s) hidden.