Fix templated pip pins ignored beside pylock (#1873)

crates/socket-patch-cli/tests/mode_migration_pypi.rs

@@ -3289,6 +3289,47 @@ async fn pip_lock_vendor_wires_requirements_with_the_pylock() {
     assert!(!root.join(format!(".socket/vendor/pypi/{UUID}")).exists());
 }
 
+/// #1873: pip expands `${NAME}` on every requirements line where it runs,
+/// so what `six==${SIXV}` installs is unknowable to vendor. Beside a `pip
+/// lock` pylock.toml, vendor refuses it and writes nothing (it used to wire
+/// only the lock, name requirements.txt an unpatched loser and exit 0),
+/// whatever this process's environment holds. A pylock-only wiring the
+/// templated line later contests fails `vendor --check`.
+#[tokio::test]
+async fn pip_lock_vendor_refuses_an_env_templated_requirements_pin() {
+    const TEMPLATED: &str = "idna==3.7\nsix==${SP_E2E_1873_SIXV}\n";
+    for env in [&[("SP_E2E_1873_SIXV", "1.16.0")][..], &[][..]] {
+        let (_tmp, root) = project();
+        stage_pip_lock(&root);
+        std::fs::write(root.join("requirements.txt"), TEMPLATED).unwrap();
+        stage_manifest(&root);
+        let before = tree(&root);
+        let (code, out) = run_cli(&root, &["vendor"], env);
+        assert_ne!(code, 0, "{env:?}: {out:#}");
+        assert!(
+            out.to_string().contains("pypi_requirement_env_templated"),
+            "{env:?}: {out:#}"
+        );
+        assert_eq!(tree(&root), before, "{env:?}: nothing written");
+    }
+
+    // A pylock-only wiring (vendored before six was named) is contested
+    // once the templated line names six.
+    let env = [("SP_E2E_1873_SIXV", "1.16.0")];
+    let (_tmp, root) = project();
+    stage_pip_lock(&root);
+    std::fs::write(root.join("requirements.txt"), "idna==3.7\n").unwrap();
+    stage_manifest(&root);
+    let (code, out) = run_cli(&root, &["vendor"], &env);
+    assert_eq!(code, 0, "{out:#}");
+    let (code, out) = run_cli(&root, &["vendor", "--check"], &env);
+    assert_eq!(code, 0, "{out:#}");
+    std::fs::write(root.join("requirements.txt"), TEMPLATED).unwrap();
+    let (code, out) = run_cli(&root, &["vendor", "--check"], &env);
+    assert_eq!(code, 1, "pip may install six from PyPI: {out:#}");
+    assert!(out.to_string().contains("contested"), "{out:#}");
+}
+
 /// #1758: a pylock vendored while requirements.txt did not pin six yet
 /// (a project vendored before the fix wired only the pylock). Once the
 /// file pins six==1.16.0, the next `vendor` wires it to the committed

crates/socket-patch-core/src/utils/requirements.rs

@@ -288,6 +288,15 @@ pub(crate) fn strip_comment(text: &str) -> &str {
     split_comment(text).0
 }
 
+/// Whether pip's [`expand_env_vars`] would substitute anything in `code`.
+/// pip expands every line, not only `-r` / `-c` targets, so `six==${SIXV}`
+/// pins whatever `SIXV` holds where pip runs: a Docker `ARG`, a CI step,
+/// none of which this process sees. Such a line pins no knowable release
+/// (#1873).
+pub(crate) fn has_env_reference(code: &str) -> bool {
+    code.contains("${") && expand_env_vars(code, |_| Some("_".to_string())) != code
+}
+
 /// pip's `expand_env_variables`: each `${NAME}` whose `NAME` is
 /// `[A-Z0-9_]+` is replaced by `lookup(NAME)`; an unset or empty variable
 /// leaves the reference as written. pip expands after stripping comments

crates/socket-patch-core/src/vendor/pypi.rs

@@ -244,6 +244,24 @@ async fn requirements_pins_target(project_root: &Path, target: Option<(&str, &st
     }
 }
 
+/// #1873: refuse wiring a lock beside a requirements set that names the
+/// target through a `${NAME}` pip expands where it runs (`six==${SIXV}`).
+/// The release it installs is unknowable here, so naming requirements.txt
+/// a loser would let the lock wiring report the package patched while pip
+/// installs it from the registry.
+async fn refuse_templated_pin(
+    project_root: &Path,
+    target: Option<(&str, &str)>,
+) -> Result<(), (&'static str, String)> {
+    let Some((name, _)) = target else {
+        return Ok(());
+    };
+    match super::pypi_requirements::templated_pin(project_root, name).await {
+        Some(refusal) => Err(refusal),
+        None => Ok(()),
+    }
+}
+
 /// Route the project to a wiring flavor, first match wins. Lockfiles are the
 /// authoritative "this tool manages installs" signal, so locks are compared
 /// with locks (precedence follows migration direction / ecosystem currency:
@@ -396,6 +414,7 @@ async fn detect_pypi_flavor(
             }
             None => false,
         };
+        refuse_templated_pin(project_root, target).await?;
         if !wired_with_lock && exists(PYPI_REQUIREMENTS).await {
             present.push(PYPI_REQUIREMENTS);
         }
@@ -449,6 +468,9 @@ async fn detect_pypi_flavor(
         }
         _ => false,
     };
+    if governing.is_some() {
+        refuse_templated_pin(project_root, target).await?;
+    }
     if governing.is_some()
         && !wired_with_lock
         && requirements_pins_target(project_root, target).await
@@ -4194,6 +4216,21 @@ mod tests {
         }
     }
 
+    /// #1873: beside a governing tool lock, a requirements pin templated
+    /// with pip's `${VAR}` refuses instead of being named a loser: the
+    /// release it installs is unknown.
+    #[tokio::test]
+    async fn governing_lock_refuses_an_env_templated_requirements_pin() {
+        let tmp = tempfile::tempdir().unwrap();
+        touch(tmp.path(), "Pipfile", "[packages]\nsix = \"==1.16.0\"\n").await;
+        touch(tmp.path(), "Pipfile.lock", "{}").await;
+        touch(tmp.path(), "requirements.txt", "six==${SIXV}\n").await;
+        let (code, detail) = detect_pypi_flavor(tmp.path(), Some(("six", "1.16.0")))
+            .await
+            .unwrap_err();
+        assert_eq!(code, "pypi_requirement_env_templated", "{detail}");
+    }
+
     /// #912: a pylock-only Pipenv checkout can't be vendored — Pipenv drops
     /// the `archive` entry the wiring writes and installs the upstream
     /// release — so it refuses with a `pipenv lock` pointer. Without a

crates/socket-patch-core/src/vendor/pypi_requirements.rs

@@ -27,8 +27,8 @@ use crate::crawlers::python_crawler::canonicalize_pypi_name;
 use crate::utils::fs::{atomic_write_bytes_preserving_mode, read_regular_to_string};
 use crate::utils::line_endings::{self, terminator};
 use crate::utils::requirements::{
-    expand_env_vars, hash_options, logical_lines, parse_requirement, requirement_name,
-    requires_hashes, shlex_split, split_comment, strip_comment, vendor_tag,
+    expand_env_vars, has_env_reference, hash_options, logical_lines, parse_requirement,
+    requirement_name, requires_hashes, shlex_split, split_comment, strip_comment, vendor_tag,
 };
 
 use super::common::refuse_symlinked;
@@ -79,6 +79,8 @@ struct PinScan {
     /// environment-marker branch; whether that is ambiguous depends on the
     /// target pins of the whole requirement set ([`marker_split_ok`]).
     other_branches: bool,
+    /// Some occurrence carries a `${NAME}` pip expands where it runs.
+    templated: bool,
 }
 
 /// Whether the other version's marker branches (if any) in a requirement
@@ -105,13 +107,16 @@ fn marker_split_ok(scans: &[PinScan]) -> bool {
 /// all carry a marker (`split_ok` from [`marker_split_ok`]).
 #[derive(Debug, PartialEq, Eq)]
 enum PinRefusal {
+    Templated,
     Extras,
     NotPinned,
     UnmarkedSplit,
 }
 
 fn pin_refusal(scan: &PinScan, split_ok: bool) -> Option<PinRefusal> {
-    if scan.extras {
+    if scan.templated {
+        Some(PinRefusal::Templated)
+    } else if scan.extras {
         Some(PinRefusal::Extras)
     } else if scan.range {
         Some(PinRefusal::NotPinned)
@@ -131,8 +136,18 @@ fn scan_pins(content: &str, canon_name: &str, version: &str) -> PinScan {
     let mut found_extras = false;
     let mut found_range = false;
     let mut other_branches = false;
+    let mut found_templated = false;
     for ll in logical_lines(content) {
         let code = strip_comment(&ll.text);
+        if has_env_reference(code)
+            && requirement_name(code).is_some_and(|name| canonicalize_pypi_name(name) == canon_name)
+        {
+            // pip expands `${NAME}` where it runs, so the release this
+            // line installs is unknowable here, and rewriting it would
+            // drop the template for every other environment (#1873).
+            found_templated = true;
+            continue;
+        }
         let Some(req) = parse_requirement(code) else {
             // A line that names the package but is no requirement pip
             // reads cleanly is never a pin to rewrite (fail closed).
@@ -184,6 +199,7 @@ fn scan_pins(content: &str, canon_name: &str, version: &str) -> PinScan {
         extras: found_extras,
         range: found_range,
         other_branches,
+        templated: found_templated,
     }
 }
 
@@ -207,7 +223,9 @@ fn find_pin(content: &str, canon_name: &str, version: &str) -> PinSearch {
     let split_ok = marker_split_ok(std::slice::from_ref(&scan));
     match pin_refusal(&scan, split_ok) {
         Some(PinRefusal::Extras) => return PinSearch::Extras,
-        Some(PinRefusal::NotPinned | PinRefusal::UnmarkedSplit) => return PinSearch::Range,
+        Some(PinRefusal::Templated | PinRefusal::NotPinned | PinRefusal::UnmarkedSplit) => {
+            return PinSearch::Range
+        }
         None => {}
     }
     match scan.exact.into_iter().next() {
@@ -446,6 +464,37 @@ pub(super) async fn sibling_pin_wirable(root: &Path, canon_name: &str, version:
         && refuse_symlinked(root, &rels).await.is_ok()
 }
 
+/// #1873: refuse a requirements line naming `canon_name` through a
+/// `${NAME}` pip expands where it runs (`six==${SIXV}`). What it installs
+/// is unknowable here, so the vendor neither rewrites it nor names the file
+/// a loser beside a lock that it wires alone.
+fn templated_refusal(rel: &str, canon_name: &str) -> (&'static str, String) {
+    (
+        "pypi_requirement_env_templated",
+        format!(
+            "{rel}: the {canon_name} requirement uses a ${{NAME}} environment variable pip \
+             expands at install time, so the release it installs is unknown; pin it \
+             literally or use agent mode (`scan --mode agent` + `socket-patch apply`) \
+             instead"
+        ),
+    )
+}
+
+/// [`templated_refusal`] for the first requirements file in the tree pip
+/// reads from `root` that names `canon_name` through a `${NAME}`.
+pub(super) async fn templated_pin(root: &Path, canon_name: &str) -> Option<(&'static str, String)> {
+    let files = collect_requirements_files(root).await.ok()?;
+    files.iter().find_map(|f| {
+        logical_lines(&f.content).into_iter().find_map(|ll| {
+            let code = strip_comment(&ll.text);
+            (has_env_reference(code)
+                && requirement_name(code)
+                    .is_some_and(|name| canonicalize_pypi_name(name) == canon_name))
+            .then(|| templated_refusal(&f.rel, canon_name))
+        })
+    })
+}
+
 /// The `requirements_line` records of a mixed entry (a Pipenv or uv entry
 /// that also wired its exported requirements file, #612 / #1368).
 pub(super) const RECORD_KIND: &str = "requirements_line";
@@ -846,6 +895,7 @@ async fn plan_requirements(
 
     for (file, scan) in files.iter().zip(scans) {
         match pin_refusal(&scan, split_ok) {
+            Some(PinRefusal::Templated) => return Err(templated_refusal(&file.rel, canon_name)),
             Some(PinRefusal::Extras) => {
                 return Err((
                     "pypi_extras_unsupported",
@@ -2171,6 +2221,56 @@ mod tests {
         }
     }
 
+    /// #1873: pip expands `${NAME}` on every line where it runs, so what
+    /// `six==${SIXV}` installs is unknowable here. Vendor refuses it with
+    /// its own code (it used to say "not pinned", or beside a pylock leave
+    /// it on PyPI while the lock wiring reported the package patched) and
+    /// never rewrites the template away, in the root or an include. A
+    /// `${...}` pip does not expand (lowercase) is no template.
+    #[tokio::test]
+    async fn refuses_an_env_templated_pin() {
+        for (root, base) in [
+            ("requests==2.31.0\nsix==${SIXV}\n", None),
+            ("six == ${SIXV}  # templated\n", None),
+            ("six==1.16.0 --hash=sha256:${SIX_SHA}\n", None),
+            ("-r base.txt\nidna==3.7\n", Some("six==${SIXV}\n")),
+        ] {
+            let tmp = write_root(root).await;
+            if let Some(base) = base {
+                tokio::fs::write(tmp.path().join("base.txt"), base)
+                    .await
+                    .unwrap();
+            }
+            let err = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA)
+                .await
+                .unwrap_err();
+            assert_eq!(
+                err.0, "pypi_requirement_env_templated",
+                "{root:?}: {}",
+                err.1
+            );
+            assert_eq!(read_root(tmp.path()).await, root, "nothing written");
+            assert!(!sibling_pin_wirable(tmp.path(), "six", "1.16.0").await);
+            assert_eq!(
+                templated_pin(tmp.path(), "six").await.map(|r| r.0),
+                Some("pypi_requirement_env_templated"),
+                "{root:?}"
+            );
+            // Another package's line is untouched by the refusal.
+            assert!(templated_pin(tmp.path(), "idna").await.is_none());
+        }
+        let tmp = write_root("requests==${REQV}\nsix==1.16.0\n").await;
+        assert!(templated_pin(tmp.path(), "six").await.is_none());
+        wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA)
+            .await
+            .unwrap();
+        assert_eq!(
+            find_pin("six==${sixv}\n", "six", "1.16.0"),
+            PinSearch::Range,
+            "a lowercase reference stays literal and is no pin"
+        );
+    }
+
     #[tokio::test]
     async fn rewrites_hash_pinned_continuation_and_preserves_crlf() {
         // A hash-pinned requirement spanning two physical lines, CRLF file.

crates/socket-patch-core/src/vex/discover/pypi_other.rs

@@ -287,6 +287,15 @@ fn requirements_line(ctx: &DiscoverCtx<'_>, file: &str, line: &str, out: &mut Di
         return;
     }
     let Some((name, location)) = requirement_location(code) else {
+        if requirements::has_env_reference(code) {
+            // pip expands `${NAME}` where it runs, so a registry line
+            // carrying one may install any release of the package it
+            // names: evidence against every wiring of it (#1873).
+            if let Some(name) = requirements::requirement_name(code) {
+                out.resolved_elsewhere_any_version(file, pypi_name_purl(name));
+            }
+            return;
+        }
         // A registry requirement; a pin that can only install one release
         // is evidence against another lock's wiring of that release.
         registry_pin(file, code, out);
@@ -1873,6 +1882,51 @@ mod tests {
         }
     }
 
+    /// REGRESSION (#1873): pip expands `${NAME}` on every requirements
+    /// line where it runs (a Docker `ARG`, a CI step), so `six==${SIXV}`
+    /// may install any release, including the wired one, from PyPI. It
+    /// contests a lock's wiring of six whatever this process's environment
+    /// holds. A lowercase `${...}` stays literal in pip and pins nothing.
+    #[tokio::test]
+    async fn issue_1873_env_templated_pin_contests_wiring() {
+        const WHL: &str = "six-1.16.0-py2.py3-none-any.whl";
+        let vendored = vendored_wheel(UUID_A, WHL);
+        let wired = pipfile_lock(serde_json::json!({ "default": {
+            "six": { "file": format!("./{vendored}"), "hashes": [format!("sha256:{SHA}")] },
+        }}));
+        let discover = |requirements: &'static str| {
+            let wired = wired.clone();
+            async move {
+                let p = Project::new();
+                p.write("Pipfile.lock", &wired)
+                    .write("Pipfile", "[packages]\nsix = \"*\"\n")
+                    .write("requirements.txt", requirements);
+                p.discover().await
+            }
+        };
+        for line in [
+            "six==${SP_TEST_1873_SIXV}\n",
+            "idna==3.7\nSix == ${SIXV}  # templated\n",
+            "six${SIX_SPEC}\n",
+            "six==1.16.0 --hash=sha256:${SIX_SHA}\n",
+        ] {
+            let out = discover(line).await;
+            assert!(out.refs.is_empty(), "{line:?}: {:#?}", out.refs);
+            let c = out
+                .vendored_contest("pkg:pypi/six@1.16.0", UUID_A)
+                .unwrap_or_else(|| panic!("{line:?}: not contested"));
+            assert_eq!(
+                c.other,
+                std::path::Path::new("requirements.txt"),
+                "{line:?}"
+            );
+        }
+        for line in ["six==${sixv}\n", "idna==${IDNAV}\n"] {
+            let out = discover(line).await;
+            assert_eq!(out.refs.len(), 1, "{line:?}: {:#?}", out.diagnostics);
+        }
+    }
+
     /// #1602: a lock tree that wires the release itself, under another
     /// PEP 440 spelling, is not a contester even when it also pins it:
     /// pip resolves the include's `==1.16.0` to the tree's own vendored