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:#}");
+}
+
 /// #1851: requirements.txt pins six only through a `-r` include outside
 /// the project root, beside the `pip lock` pylock.toml. `pip install -r
 /// requirements.txt` installs that pin, and vendor can't edit the

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

@@ -263,6 +263,24 @@ async fn refuse_pin_outside_root(
     }
 }
 
+/// #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:
@@ -416,6 +434,7 @@ async fn detect_pypi_flavor(
             None => false,
         };
         refuse_pin_outside_root(project_root, target).await?;
+        refuse_templated_pin(project_root, target).await?;
         if !wired_with_lock && exists(PYPI_REQUIREMENTS).await {
             present.push(PYPI_REQUIREMENTS);
         }
@@ -471,6 +490,7 @@ async fn detect_pypi_flavor(
     };
     if governing.is_some() {
         refuse_pin_outside_root(project_root, target).await?;
+        refuse_templated_pin(project_root, target).await?;
     }
     if governing.is_some()
         && !wired_with_lock
@@ -4349,6 +4369,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;
@@ -83,6 +83,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
@@ -109,13 +111,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)
@@ -136,15 +141,25 @@ fn scan_pins(content: &str, canon_name: &str, version: &str) -> PinScan {
     let mut exact_extras = false;
     let mut found_range = false;
     let mut other_branches = false;
+    let mut found_templated = false;
     // Hosted mode also rewrites a file's one line for the package when it
     // is unpinned (`six[foo]`), so that extras line counts as exact too.
     let mut named_lines = 0usize;
     let mut unpinned_extras = false;
     for ll in logical_lines(content) {
         let code = strip_comment(&ll.text);
-        if requirement_name(code).is_some_and(|name| canonicalize_pypi_name(name) == canon_name) {
+        let names_target =
+            requirement_name(code).is_some_and(|name| canonicalize_pypi_name(name) == canon_name);
+        if names_target {
             named_lines += 1;
         }
+        if has_env_reference(code) && names_target {
+            // 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).
@@ -202,6 +217,7 @@ fn scan_pins(content: &str, canon_name: &str, version: &str) -> PinScan {
         exact_extras: exact_extras || (unpinned_extras && named_lines == 1),
         range: found_range,
         other_branches,
+        templated: found_templated,
     }
 }
 
@@ -225,7 +241,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() {
@@ -465,6 +483,22 @@ 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"
+        ),
+    )
+}
+
 /// The out-of-root `-r` include (`../shared/base.txt`) through which pip
 /// installs `canon_name==version` from somewhere other than a vendored
 /// wheel, if the requirements set has one (#1851). Vendor cannot edit that
@@ -577,6 +611,21 @@ fn extras_refusal(rel: &str, canon_name: &str) -> (&'static str, String) {
     )
 }
 
+/// [`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))
+        })
+    })
+}
+
 /// [`sibling_pin_wirable`] for a lock about to be wired fresh, except
 /// that a requirements set pinning exactly `canon_name==version` with
 /// extras (`six[foo]==1.16.0`) refuses the package, as the
@@ -1027,6 +1076,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(extras_refusal(&file.rel, canon_name)),
             Some(PinRefusal::NotPinned) => {
                 return Err((
@@ -2431,6 +2481,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_locks.rs

@@ -1420,10 +1420,16 @@ mod tests {
             ("-e git+https://example.com/click.git#egg=click\n", true),
             ("git+https://example.com/click.git#egg=click\n", true),
             ("../wheels/click-8.1.7-py3-none-any.whl\n", true),
+            // #1873: pip expands `${NAME}` where it installs, so a
+            // templated pin may install the wired release from PyPI.
+            ("click==${CLICK_VERSION}\n", true),
+            ("click${CLICK_SPEC}\n", true),
             (
                 "click @ https://files.pythonhosted.org/packages/click-8.1.6-py3-none-any.whl\n",
                 false,
             ),
+            ("click==${click_version}\n", false),
+            ("idna==${IDNA_VERSION}\n", false),
             ("click==8.1.6\n", false),
             ("idna==3.7\n", false),
         ] {