Skip to content

Fix Pipenv venv discovery settings view (#645, #546) - #654

Open
Mikola Lysenko (mikolalysenko) wants to merge 10 commits into
mainfrom
agent/fix-pipenv-venv-settings-view
Open

Mikola Lysenko (mikolalysenko) wants to merge 10 commits into
mainfrom
agent/fix-pipenv-venv-settings-view

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #645
Fixes #546

Summary

Agent mode, and the hosted stale-install check, now find the venv a Pipenv project actually uses in two cases they used to miss. In both, socket-patch fell through to the system interpreter, patched that instead, and let vex attest not_affected while the venv Pipenv runs stayed vulnerable.

Root cause

pipenv_project_site_packages (crates/socket-patch-core/src/crawlers/python_crawler.rs) chose the venv from settings that don't match what the installed Pipenv sees:

Fix

  • The dotenv file is read once using a nonblocking, regular-file-only read. The modern parser follows native record boundaries, quoted keys/values, multiline and escape rules, invalid-record recovery and interpolation; a separate legacy parser follows the 2018 final-map and process-first interpolation rules.
  • Discovery preserves the modern dotenv and process-environment views, then includes the two native-proven cached shell profiles. The 2018 shell marks itself active before placement; the 2020 shell still reads a dynamic active prefix with cached Project flags. Dynamic path expansion stays separate from cached project identity, and results remain deduplicated and limited to project environments.
  • When a ./.venv directory exists, any setting other than an explicit "in project" now returns both the WORKON_HOME venv and ./.venv, so the venv any Pipenv release uses gets patched.
  • docs/testing/pipenv-compatibility.md describes the new rules.

Known over-approximation: if .env says "in project" and the project also has a WORKON_HOME venv, that venv is patched too, because the environment-only view still returns it. It belongs to the same project, so this is harmless.

Verification against real Pipenv

pipenv --venv, with both a ./.venv directory and $WORKON_HOME/custom present:

scenario Pipenv 2023.10.24 Pipenv 2026.0.3 discovery (this PR)
#645: PIPENV_VENV_IN_PROJECT=0 p/.venv wh/custom both, WORKON_HOME first
#546: .env PIPENV_CUSTOM_VENV_NAME=custom (no .venv) wh/custom wh/custom wh/custom

Tests (red → green)

Per issue:

I ran the new tests on top of main first, with the parser stubbed to return nothing. dotenv_parsing_follows_python_dotenv, pipenv_dotenv_settings_move_the_venv and pipenv_venv_in_project_settings_decide_about_dot_venv FAILED. With the fix, all 63 python_crawler tests pass. I changed the existing #334 tests that asserted "explicit false skips ./.venv" to expect the corrected behaviour. The guard against a stray venv/ directory is kept.

Local runs:

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test --workspace --all-features --no-fail-fast: all passed except 12 fault-injection tests (*_write_failure_*, *unremovable*, relax_loop_must_not_traverse_symlinked_root). They depend on chmod and fail only because the sandbox runs as root. None of them touches Python discovery, and CI runs them as non-root.
  • cargo fmt --all -- --check already fails on main (~500 diffs; CI has no fmt step). The lines this PR adds are rustfmt-clean, and I didn't reformat unrelated code.
  • CI on bf91b10: all 335 checks green (6 skipped by design). That includes test (windows-latest), which first failed on a Windows-only bug in the new test's fixture, fixed in bf91b10. Bugbot's one finding (backslash escapes in quoted Windows paths) is fixed and resolved, and its re-review found no new issues.
  • No wrapper changes (npm/, pypi/, gem/) are needed, since discovery lives in core.

🤖 Generated with Claude Code


Note

Medium Risk
Changes which Python site-packages get scanned and patched for Pipenv projects; incorrect discovery could miss vulnerabilities or target the wrong venv, but scope is limited to Pipenv discovery logic with extensive regression tests.

Overview
Pipenv venv discovery now mirrors how Pipenv actually picks an environment, fixing agent scans and hosted stale-install checks that used to fall through to the wrong interpreter (#546, #645).

Discovery runs earlier for Pipenv projects and no longer relies on process env alone. It reads the project .env (or PIPENV_DOTENV_LOCATION, unless PIPENV_DONT_LOAD_ENV) with a python-dotenv–style parser plus a 2018 legacy parser, then unions several native timing profiles (current dotenv + process, and cached 2018/2020 shell views) so WORKON_HOME, PIPENV_CUSTOM_VENV_NAME, active VIRTUAL_ENV, and in-project flags match what different Pipenv releases see. Relative WORKON_HOME is resolved from the project directory (not the CLI process cwd).

For #645, an explicit “not in project” no longer universally drops ./.venv: when both WORKON_HOME and ./.venv exist, both are scanned (WORKON_HOME first) because only Pipenv 2023.11.14+ ignores .venv in that case; older releases still use it.

Tests expand scan-level, redirect/VEX, and python_crawler unit coverage for dotenv syntax, legacy shell WORKON_HOME, and the revised in-project behavior. docs/testing/pipenv-compatibility.md documents the new settings views and version-dependent ./.venv behavior.

Reviewed by Cursor Bugbot for commit 3ceb956. Configure here.

Review follow-up on d8356ae2: all three reported discovery findings are corrected. Modern dotenv binding/interpolation and per-view active-environment selection are preserved. Native-proven 2018 and 2020 shell profiles now retain their own parser, cached settings and active-prefix timing; discovery includes their project environments without scanning unrelated venvs. Verified with 162 repository tests, seven hosted stale-install regression cases, 18 real native environment cases, 34 legacy and 58 modern parser cases, targeted Clippy and independent review. Ready to merge as-is from this review. 335 successful checks, 7 skipped, and 8 successful workflows (1 additional workflow skipped). Bugbot is clear on this commit; no unresolved review threads or new actionable findings.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Agent mode and hosted stale-install checks now find the venv Pipenv
really uses in two cases where they used to miss it, patch the system
interpreter instead, and let VEX attest not_affected:

- WORKON_HOME, PIPENV_CUSTOM_VENV_NAME or PIPENV_VENV_IN_PROJECT set in
  the project's .env (or PIPENV_DOTENV_LOCATION), which every Pipenv
  command loads before it picks the venv (#546).
- An explicit "not in project" setting next to a ./.venv directory.
  Only Pipenv 2023.11.14+ skips ./.venv then; 2018.11 to 2023.10.24
  still use it, so both venvs are now patched (#645).

Assisted-by: Claude Code:claude-opus-5-5
Scan-level regressions for both fixes: a .env-named venv is scanned
(and PIPENV_DONT_LOAD_ENV turns that off), and an explicit "not in
project" setting now scans ./.venv as well as the WORKON_HOME venv.
The Pipenv compatibility doc describes the new discovery rules.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 3, 2026 05:04
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-cli/tests/in_process_python_envs.rs
The new .env test removed the whole WORKON_HOME on Windows, where
site-packages sits one level shallower, so the .env-named venv went
with it. .env values now decode exactly python-dotenv's escapes, so a
backslash in a quoted Windows path is kept as written.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@cursor

cursor Bot commented Oct 3, 2026

Copy link
Copy Markdown

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Quoted Windows paths break dotenv tests
    • Escaped backslashes in Windows paths when writing to double-quoted .env values to prevent parse_dotenv from interpreting them as escape sequences.

Create PR

Or push these changes by commenting:

@cursor push dec35ec829
Preview (dec35ec829)
diff --git a/crates/socket-patch-cli/tests/in_process_python_envs.rs b/crates/socket-patch-cli/tests/in_process_python_envs.rs
--- a/crates/socket-patch-cli/tests/in_process_python_envs.rs
+++ b/crates/socket-patch-cli/tests/in_process_python_envs.rs
@@ -689,7 +689,7 @@
         project.join(".env"),
         format!(
             "export WORKON_HOME=\"{}\"\nPIPENV_CUSTOM_VENV_NAME=proj-env # named\n",
-            workon.display()
+            workon.display().to_string().replace('\\', "\\\\")
         ),
     )
     .unwrap();

diff --git a/crates/socket-patch-core/src/crawlers/python_crawler.rs b/crates/socket-patch-core/src/crawlers/python_crawler.rs
--- a/crates/socket-patch-core/src/crawlers/python_crawler.rs
+++ b/crates/socket-patch-core/src/crawlers/python_crawler.rs
@@ -3545,9 +3545,10 @@
             "HOME",
             tmp.path().join("home").to_string_lossy().into_owned(),
         )]);
+        let base_escaped = base.replace('\\', "\\\\");
         for dotenv in [
             format!("WORKON_HOME={base}/elsewhere\n"),
-            format!("# venvs\nexport WORKON_HOME=\"{base}/elsewhere\"  # here\n"),
+            format!("# venvs\nexport WORKON_HOME=\"{base_escaped}/elsewhere\"  # here\n"),
             format!("WORKON_HOME='{base}/elsewhere'\n"),
             format!("BASE={base}\nWORKON_HOME=${{BASE}}/elsewhere # comment\n"),
         ] {

You can send follow-ups to the cloud agent here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 3, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review — head bf91b102859907d613f0bb38c60b0f14dd6c3037.

  • CI: all checks green on the head (329 success, rest skipped; 0 failing or pending).
  • Bugbot: reviewed bf91b10, no new issues. Its one earlier thread (quoted Windows paths in the dotenv tests) is resolved.
  • Mergeable with no conflicts. A human approval is still required.

Reviewers: the change is in Pipenv venv discovery. It now reads .env in addition to the process environment, and it no longer treats an explicit PIPENV_VENV_IN_PROJECT=0 as meaning the same thing on every Pipenv version.


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 3, 2026
@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator Author

Codex follow-up review of d8356ae2df83d1671c49efca6c22a5a893e6e79c: all three reported findings are fixed; ready to merge as-is from this review.

The first fixes preserve native modern dotenv bindings and interpolation, including single-quoted and multiline values, and apply the active-environment decision within each settings view. Relative and empty active paths retain the verified behavior.

The remaining compatibility gap is now corrected with explicit Pipenv 2018.11.26 and 2020.11.15 shell profiles. The 2018 parser resolves the complete file mapping with process values first and its shell sets PIPENV_ACTIVE before placement. The 2020 parser uses modern interpolation but its shell retains cached Project flags and reads the active prefix before setting that marker. The correction preserves those differences, separates dynamic root expansion from cached project identity, and keeps discovery limited to the project's environments.

Verified on the exact committed source:

  • 162 repository tests passed: 69 Python unit, 59 crawler end-to-end, 19 CLI environment, seven Poetry redirect and eight Pipenv redirect tests.
  • Seven hosted stale-install regression cases require exit 1, a stale-install warning and no VEX output. The two newly added legacy cases failed before the correction by reporting success and emitting VEX; they now pass while preserving installed bytes and the Pipfile.
  • The public crawler finds all expected environments in 18 real native cases, covering the newer releases and eight older-shell cases. Independent parser comparisons pass 34 legacy and 58 modern cases; the modern corpus also agrees with the inspected 2020/2021/2022 vendored sources.
  • Independent review, targeted core/CLI Clippy, changed-region formatting and diff checks are clear. Clippy retains only the documented allowance for the pre-existing macOS unused_variables warning. The commit matches the tested hashes and merges cleanly with main 045d7ec7.

335 successful checks, 7 skipped, and 8 successful workflows (1 additional workflow skipped). Bugbot is clear on this commit; no unresolved review threads or new actionable findings. Ready for review has been restored. GitHub still requires the normal human approval before merge.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review

Please review the dotenv parsing and per-view active-environment correction on a416025dcc28d083c114ef24e7c2b09128ed3302.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review

Please review the native Pipenv 2018/2020 cached-shell discovery correction on d8356ae2df83d1671c49efca6c22a5a893e6e79c.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 3, 2026
Resolve the find_local_venv_site_packages_with conflict by keeping this
branch's early Pipenv return (Pipenv's VIRTUAL_ENV decision now lives in
pipenv_project_site_packages per settings view) and taking main's new
poetry_project_site_packages flow; the non-Poetry VIRTUAL_ENV branch no
longer needs a Pipenv guard since Pipenv projects return earlier.

Co-Authored-By: Claude <noreply@anthropic.com>
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

1 similar comment
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/crawlers/python_crawler.rs

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/crawlers/python_crawler.rs
mikolalysenko and others added 3 commits October 5, 2026 14:48
Brings in the vex_consumed alias test fix (#849) that the red
test/test-release/coverage jobs were waiting on.

Co-Authored-By: Claude <noreply@anthropic.com>
Main's spawn_env_hygiene ratchet (#850) rejects new bare binary
spawns; the two dotenv-view spawns in in_process_redirect_pipenv.rs now
start from the shared hermetic builder and keep their own PIPENV_* and
venv scrubs on top.

Co-Authored-By: Claude <noreply@anthropic.com>
The modern dotenv and process settings views passed WORKON_HOME through
unjoined, so a relative value resolved against socket-patch's own cwd
instead of the project Pipenv runs from (the legacy cached view already
joined it). Under --cwd that missed the project's venv. Join it to the
project directory in the shared helper, as the legacy path does.

Co-Authored-By: Claude <noreply@anthropic.com>
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 3ceb956. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 5, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 3ceb956.

  • CI: 335 success + 6 skipped, 0 failing; mergeable clean against main.
  • Bugbot: Cursor Bugbot check passed on 3ceb956; 0 unresolved review threads.
  • Linked issue(s) still open and not fixed on main.

Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

3 participants