repair pre-commit hooks to use project venv instead of pre-commit hook's venv

[MarcBresson]

Pull Request Check List

Resolves: #10934 and #9393

I tested the hook locally onto my project in which I raised #10934, and it worked as expected.

• Added tests for changed code.
• Updated documentation for changed code.

There is no need to update the documentation :

• I don't see any changelog to fill
• this is a bug fix

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The check for os.environ.get("PRE_COMMIT") == "1" is quite strict; consider treating any non-empty PRE_COMMIT value as enabled to be more robust against variations in how the variable is set.
• In the new test, you patch subprocess.check_output directly; if env_manager imports check_output differently (e.g. from subprocess import check_output), this patch may not be effective, so it would be safer to patch the symbol on the env_manager module instead.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The check for `os.environ.get("PRE_COMMIT") == "1"` is quite strict; consider treating any non-empty `PRE_COMMIT` value as enabled to be more robust against variations in how the variable is set.
- In the new test, you patch `subprocess.check_output` directly; if `env_manager` imports `check_output` differently (e.g. `from subprocess import check_output`), this patch may not be effective, so it would be safer to patch the symbol on the `env_manager` module instead.

## Individual Comments

### Comment 1
<location path="tests/utils/env/test_env_manager.py" line_range="637-628" />
<code_context>
+    in_project_venv_dir: Path,
+    mocker: MockerFixture,
+) -> None:
+    mocker.patch.dict(
+        os.environ,
+        {"PRE_COMMIT": "1", "VIRTUAL_ENV": "/environment/prefix"},
+        clear=False,
+    )
+    mocker.patch(
+        "subprocess.check_output",
+        side_effect=check_output_wrapper(),
+    )
+
+    env = manager.get()
+
+    assert env.path == in_project_venv_dir
+    assert env.base == Path(sys.base_prefix)
+
+
</code_context>
<issue_to_address>
**nitpick (testing):** Make the test assertion more explicit that the external VIRTUAL_ENV is ignored.

Since `VIRTUAL_ENV` is set to `/environment/prefix`, the test currently only *implicitly* checks that this value is ignored via `assert env.path == in_project_venv_dir`. To make that intent explicit and protect against regressions, please also assert that the external prefix is not used, e.g. `assert str(env.path) != "/environment/prefix"`. This will make the failure mode clearer if the external virtualenv ever becomes preferred again.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[dimbleby]

This doesn't look right. Eg not everyone uses in-project venv.

Isn't language: system - per #10574 - the way that pre-commit supports "don't use a special pre-commit only virtual environment"?

[MarcBresson]

This doesn't look right. Eg not everyone uses in-project venv.

Isn't language: system - per #10574 - the way that pre-commit supports "don't use a special pre-commit only virtual environment"?

I see language: system as a workaround more than the best solution, though, again, I have little knowledge about poetry. Forcing language: system onto users means less reproductibility. I also checked uv pre-commit hook config and they run inside pre-commit virtual env.

If my solution of forcing the project's virtual env is not the right one, maybe we could find a better one. For instance, when PRE-COMMIT en var is detected, we could enter the if block at L221. We would then have regular env resolution. The downside of this approach is if the user wants to use language: system and runs poetry from their virtual env, but it seems very unlikely.

I have updated the MR to the new proposed solution, let me know what you think

[MarcBresson]

a more deterministic proposal is to have a new option (not necessarily documented) similar to uv --no-active, to indicate we don't want to use the current active virtual env (which would be pre-commit's one).

-- EDIT --
I ended up implementing this one as it's cleaner and more flexible

[dimbleby]

I see language: system as a workaround more than the best solution

Why?

Surely simpler just to use pre-commit's supported mechanism to not-use-a-precommit-env, rather than allowing pre-commit to create that unwanted environment and then teaching poetry to ignore it.

[MarcBresson]

Indeed language: system works

For reference, this is the official documentation on the option and the rationale behind it

System hooks provide a way to write hooks for system-level executables which don't have a supported language above (or have special environment requirements that don't allow them to run in isolation such as pylint).

This hook type will not be given a virtual environment to work with – if it needs additional dependencies the consumer must install them manually.

I can change the MR to just use the language: system trick