Skip to content

Fix templated pip pins ignored beside pylock (#1873) - #1874

Open
Mikola Lysenko (mikolalysenko) wants to merge 7 commits into
mainfrom
agent/fix-pypi-requirement-env-expand
Open

Mikola Lysenko (mikolalysenko) wants to merge 7 commits into
mainfrom
agent/fix-pypi-requirement-env-expand

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1873

Summary

A pip requirement written with an environment variable (six==${SIXV})
beside a pip lock pylock.toml left six on PyPI. Vendor wired only the
lock and exited 0, vendor --check passed, and vex attested
not_affected while pip install -r requirements.txt installed the
unpatched release. Such a line now fails closed:

  • vendor refuses it with pypi_requirement_env_templated and writes
    nothing. That applies in the root file or an include, and beside a
    pylock / Pipfile.lock / uv.lock / poetry.lock instead of naming
    requirements.txt an unpatched pypi_multiple_lockfiles loser. Without
    a lock it used to say the misleading pypi_requirement_not_pinned.
  • VEX discovery / vendor --check read it as possibly installing any
    release of the package from PyPI, so it contests every wiring of that
    package (vendor --check exits 1, VEX drops the attestation).

Root cause

pip runs expand_env_variables on every requirements line before parsing
it. socket-patch only knew ${NAME} for -r/-c include targets, so the
planner (scan_pins) and discovery (pypi_other::requirements_line)
read six==${SIXV} as no pin at all, and nothing contested the
pylock-only wiring.

Design choice: refuse, don't expand

The first commit expanded the line from this process's environment.
/code-review high showed why that's wrong: pip expands where it
installs, often in a Docker ARG or CI step this process never sees.
So vendor --check would pass wherever the variable is unset, and a
vendor rewrite would freeze one environment's value into the file. The
final fix is env-independent. A line naming the package through a
pip-expandable ${NAME} (pip's [A-Z0-9_]+ grammar) is "unknown
release": the planner refuses it and discovery records any-version
evidence (resolved_elsewhere_any_version, as for editable installs).
This is the issue's "at minimum" option, and it's the only one that's
sound when the check runs somewhere other than the install.

Code-review findings, each with how it was handled:

Tests (red → green)

Issue Test Without fix With fix
#1873 planner vendor::pypi_requirements::tests::refuses_an_env_templated_pin (root, include, --hash=${..}, lowercase stays literal) FAILED ok
#1873 VEX/check vex::discover::pypi_other::tests::issue_1873_env_templated_pin_contests_wiring FAILED ok
#1873 CLI mode_migration_pypi::pip_lock_vendor_refuses_an_env_templated_requirements_pin: vendor refuses beside a pip-lock pylock with and without the var set; a pylock-only wiring the templated line contests fails vendor --check FAILED ok

The "without fix" column is the same tests run with has_env_reference stubbed to false.

Local runs:

  • cargo fmt --all -- --check: ok.
  • cargo clippy --workspace --all-features -- -D warnings: ok.
  • cargo test -p socket-patch-core --all-features --lib: 6996 passed, 4 failed. All 4 are pre-existing read-only-directory tests (copy_tree, vlt_heal, pypi_poetry / pypi_requirements write-failure) that can't fail as root in this container. They're unrelated to this diff.
  • cargo test -p socket-patch-cli --all-features --test mode_migration_pypi --test in_process_vendor: 147 + 75 passed.
  • The full workspace and the real-pip e2e suites didn't fit this container's disk allowance, so they're left to CI.

A final reviewer pushed d97f89c, which adds governing_lock_refuses_an_env_templated_requirements_pin to cover the refusal's Pipfile.lock / governing-lock branch.

Known gaps outside #1873, left for follow-up:

  • A line whose name or whole requirement is templated (${PKG}==1.0, ${SIX_REQ}) has no readable name, so it is neither refused nor counted as evidence.
  • A non-UTF-8 root requirements.txt still falls back to the pypi_multiple_lockfiles loser warning.
  • A ${TOKEN} credential in the target's direct-reference URL is refused with pypi_requirement_env_templated. That's fail-closed, but the "release it installs is unknown" wording fits it loosely.

Wrappers (npm/, pypi/, gem/) only dispatch to the binary, so they need no change.

https://claude.ai/code/session_011f9aePT5mRBMMA1kpcGZBU


Note

Medium Risk
Changes PyPI vendor routing, requirements scanning precedence, and VEX contest logic; behavior is fail-closed but affects lock + requirements.txt projects.

Overview
Fixes #1873: requirements lines like six==${SIXV} are no longer treated as absent pins when a pylock or other governing lock is wired.

Vendor now fails closed with pypi_requirement_env_templated instead of wiring only the lock, exiting 0, and leaving requirements.txt as an unpatched install path. That applies to requirements-only wiring, sibling pins beside pylock / Pipfile.lock / uv.lock, and includes. Detection uses new has_env_reference (pip’s ${A-Z0-9_}+ grammar only; lowercase ${...} stays literal).

VEX / vendor --check treat those lines as evidence the package may still install any release from PyPI (resolved_elsewhere_any_version), so an existing lock-only wiring is contested once a templated line names the package.

The tool does not expand variables from this process’s environment—install-time expansion is unknowable here.

Reviewed by Cursor Bugbot for commit eb89ff0. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
pip expands ${NAME} environment variables on every requirements line
before parsing it, so `six==${SIXV}` with SIXV=1.16.0 installs exactly
six 1.16.0. socket-patch expanded them only in -r/-c include targets,
so vendored scans and VEX discovery read the templated line as no pin.

Beside a pylock.toml that meant vendor wired only the lock, left the
line on PyPI and exited 0; `vendor --check` then verified the wiring
and `vex` attested not_affected while `pip install -r` installed the
unpatched release. Without a lock, vendor refused the line as "not
pinned".

Requirement lines are now expanded from the process environment the
way pip does, in both the vendored planner and VEX discovery. A
templated exact pin is wired like a literal one, and revert restores
the templated line as written. An unset or empty variable stays
literal, as in pip, and pins nothing.

Fixes #1873

Assisted-by: Claude Code:claude-opus-5-5
Covers #1873 end to end through the CLI: vendor wires a
`six==${VAR}` line together with the pylock.toml, `vendor --check`
is green, revert restores the templated line, and a pylock-only
wiring the templated pin contests fails `vendor --check`.

Assisted-by: Claude Code:claude-opus-5-5
pip expands ${NAME} where it installs (a Docker ARG, a CI step), which
socket-patch can't see. Reading `six==${SIXV}` through this process's
environment lets `vendor --check` pass wherever SIXV is unset, and lets
vendor rewrite the template away for every other environment.

So a templated requirement now fails closed instead:

- vendor refuses it with pypi_requirement_env_templated and writes
  nothing, in the root file or an include, also beside a pylock.toml
  or tool lock instead of naming requirements.txt an unpatched loser;
- VEX discovery and `vendor --check` read it as possibly installing
  any release of the package from PyPI, so it contests every wiring
  of that package and VEX no longer attests it.

A lowercase ${...} stays literal in pip and is still no pin.

Fixes #1873

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

1 similar comment
@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.

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

Copy link
Copy Markdown
Collaborator Author

Ready for review at 28a6249 (burn-down agent).

  • CI: 82/82 checks green on the head (32 success, 50 skipped, no failures). No main-wide failures.
  • Bugbot: Cursor Bugbot check passed on this head; no open review threads.
  • Mergeable with no conflicts; no CHANGELOG.md change.

Slack announcement not sent this run (the Slack send tool was unavailable); a later run will retry.


Generated by Claude Code

refuse_templated_pin runs in two branches of detect_pypi_flavor, the
pylock branch and the governing-lock branch. Only the pylock branch had
a test: with the governing-lock call removed, every core and CLI test
still passed. Add a Pipfile.lock project with requirements.txt pinning
six==${SIXV}, which must refuse with pypi_requirement_env_templated.
It fails without that call.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011f9aeS41XjBLw8LQsUUTYU
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 30, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run

Final reviewer: I pushed d97f89c.

The new test. refuse_templated_pin runs in two branches of detect_pypi_flavor, and only the pylock branch had a test. When the governing-lock call (pypi.rs:472) was removed, every core lib and mode_migration_pypi test still passed. The new test, governing_lock_refuses_an_env_templated_requirements_pin, sets up a Pipfile.lock project whose requirements.txt has six==${SIXV}. It passes with the call and fails without it.

Two non-blocking gaps the description could list. Both are outside #1873:

  • A line whose name or whole requirement is templated (${PKG}==1.0, ${SIX_REQ}) has no readable name, so it is neither refused nor counted as evidence.
  • A non-UTF-8 root requirements.txt falls back to the old loser warning.

A wording mismatch. A ${TOKEN} credential in the target's direct-reference URL is now refused with the templated code. That is fail-closed, but the message "the release it installs is unknown" fits this case a little loosely.

I removed Ready for review until CI and Bugbot are green on the new head.


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.

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Final reviewer: claim released. d97f89c from an earlier final-review run already covers the governing-lock refusal I was going to test, so I pushed nothing. My separate review of 28a6249 found nothing that blocks merging.


Generated by Claude Code

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

Copy link
Copy Markdown
Collaborator Author

Ready for review at d97f89c (burn-down agent).

  • CI: 50/50 checks green on the head (31 success, 19 path-skipped), no failures.
  • Bugbot: reviewed d97f89c, no new issues; no unresolved review threads.
  • Mergeable, no conflicts; no CHANGELOG.md change.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief (d97f89c)

What it does. pip expands ${VAR} on every line of a requirements file, so a line like six==${SIXV} pins whatever release that variable names on the installing machine. socket-patch read such a line as no pin. Beside a pip-lock pylock.toml it wired only the lock, and vendor --check and vex then reported the patch as effective while pip installed the unpatched release (#1873). Now the planner refuses a line that names the patched package through a variable, with the new code pypi_requirement_env_templated, and writes nothing. VEX discovery and vendor --check treat that line as possibly installing any release.

Risk: low. This is fail-closed and narrow. The new check only looks for a variable reference using pip's own [A-Z0-9_]+ grammar. It never reads the environment, and the refusal message holds only the relative path and the package name.

Look here

Verified

Changes I made: none.

Open questions (worth a conscious OK, not blocking)

Auto-merge is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 1, 2026
Resolve pypi_requirements.rs against #1796 (extras pin beside a pylock):
scan_pins keeps both the ${VAR} templated-pin refusal and main's
named_lines/exact_extras tracking (a templated line still counts as a
line naming the package), and both templated_refusal/templated_pin and
main's extras_refusal/fresh_sibling_pin are kept.

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 eb89ff0. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: merged main (cbbbdb9) to clear the conflict and re-labeled Ready for review.

  • Head: eb89ff00c21f1fcda9df555e0832a6a326c46c1b, mergeable.
  • Conflict: vendor/pypi_requirements.rs against Fix extras pin un-hosted beside a pylock (#1794) #1796 (extras pin beside a pylock). scan_pins now keeps both the ${VAR} templated-pin refusal and main's named_lines/exact_extras tracking; a templated line still counts as a line naming the package. Both templated_refusal/templated_pin and main's extras_refusal/fresh_sibling_pin are kept, and the match arm uses the shared extras_refusal.
  • Local: cargo check -p socket-patch-core --all-targets clean; mode_migration_pypi 77/77; core pypi lib tests pass except two permission-based tests that fail only when run as root (the sandbox is root).
  • CI: 31/31 ran green (19 skipped by path filters).
  • Bugbot: reviewed this head, no new issues.
  • Auto-merge left armed (only a merge from base was pushed).
  • Slack announcement could not be sent this run (send tool unavailable); next run retries.

Generated by Claude Code

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

2 participants