Skip to content

Fix Pipenv stale-install remedy built twice (#1878) - #1883

Merged
Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-pipenv-stale-remedy
Nov 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-pipenv-stale-remedy

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 #1878

Summary

When an unpatched release is still installed, Pipenv won't reinstall it.
Hosted scan and vendored pypi both tell the user what to run, and now
they word that remedy through one core helper,
vendor::pypi_pipenv::stale_install_detail.

Root cause

The remedy was built twice, in scan/hosted/python.rs and in
vendor/pypi.rs::pipenv_stale_install_warning. Each copy sorted the stale
sites into the same three classes (inherited base site, a venv copy hiding
the base one, the venv's own copy), and the two copies drifted. Hosted
hand-split the purl, fell back to a literal <package>, and told the user to
run socket-patch vex --product <purl> with the placeholder left in.

Changes

  • Core helper. stale_install_detail(lock, name, version, sites, inherited, lock_change) partitions the sites and words each class once.
    join_dirs is the one directory-list formatter.
  • Vendored. pipenv_stale_install_warning calls the helper, and its text
    is byte-identical. The existing vendor/pypi.rs pipenv stale tests guard
    this.
  • Hosted. Hosted calls the helper per site and keeps its warning code,
    its lead sentence, and the word "rewritten". It reads the package name
    with purl_parts, falling back to the bare name for a purl with no
    version.
  • Name guard. A name that isn't a distribution name prints as
    <package>, so a decoded purl can't put shell metacharacters into the
    commands users copy.

A deviation from the issue

The issue suggested substituting the package purl into
socket-patch vex --product <purl>. That would be wrong. --product takes
the project's product purl, not the patched package's (see
docs/testing/pipenv-compatibility.md). Both paths now end with
socket-patch vex, the command vendored and CLI_CONTRACT.md already use.
vex itself says when a product must be passed.

Not changed (review notes)

  • Hosted still emits one warning per (site, purl), and vendored one per
    purl. The issue's "hosted keeps its own warning code and lead sentence"
    covers this, and changing warnings[] cardinality is a contract change.
  • commands/pypi_reinstall.rs keeps its rollback sentence. It is a
    different message ("restored lock" after an unwind), it already uses
    core's stale_install_remedy, and its own purl split is the A14 cleanup.

Tests

Issue Test Red → green
#1878 hosted wording scan::hosted::python::tests::pipenv_remedy_is_the_shared_core_wording (equals the helper's text, no <purl>/<package>, --dev and --categories "docs" reach hosted) failed on main's hosted code, passes now
#1878 three site classes vendor::pypi_pipenv::tests::stale_install_detail_words_each_site_class (own, shadowing, inherited, the mixed For <dirs>: lead, unsafe name) new helper
vendored text unchanged existing vendor::pypi pipenv stale tests pass

The CLI test that re-tested core's category logic is folded into the hosted
test above.

Commands run locally:

  • cargo fmt --all -- --check and cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-cli --all-features --lib --test in_process_redirect_pipenv: 940 + 11 passed.
  • cargo test -p socket-patch-core --all-features --lib: 6995 passed and 4 failed. The 4 failures are copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_* and pypi_requirements::wire_failure_rolls_back_*. They make dirs read-only with chmod, which root ignores (this container runs as uid 0), and they are in files this PR doesn't touch.
  • The full workspace test build ran out of disk in this container, so CI covers the integration suites.

/code-review high ran on the diff. Its confirmed findings are fixed in
d35ba44: the hosted lock wording, the name fallback, the name guard,
purl_parts reused instead of widening parse_pypi_purl, the hosted test
coverage, and the duplicate dir join.

https://claude.ai/code/session_011f9aeRsZjPSuB7GuzgvqyS


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Hosted scan and vendored pypi each told the user how to reinstall a
release Pipenv kept installed, and the copies drifted: hosted asked
for `socket-patch vex --product <purl>` with the placeholder left in,
hand-split the purl and fell back to a literal `<package>`.

Both now call core's pypi_pipenv::stale_install_detail, which sorts
the stale sites into the three classes (inherited base site, a venv
copy hiding the base one, the venv's own copy) and words each remedy.
Vendored text is unchanged; hosted keeps its warning code and lead
sentence, names the package via parse_pypi_purl, and ends with the
runnable `socket-patch vex` that vendored already used. `--product`
takes the project's product purl, not the patched package's, so
filling in the package purl would have been wrong.

Fixes #1878

Assisted-by: Claude Code:claude-opus-5-5
Review follow-ups for the shared Pipenv remedy: hosted again says the
"rewritten" Pipfile.lock (vendor says "wired"), a purl without a
version falls back to the bare package name instead of the whole purl,
and a name that is not a distribution name prints as `<package>`
rather than landing unquoted in the shell commands users copy. The
hosted test checks the named-category arguments again, and both
callers share one directory-list formatter.

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


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

Burn-down agent: labeled Ready for review at d35ba44d.

  • CI: 82/82 check runs green on this head; mergeable with no conflicts; no CHANGELOG.md change.
  • Bugbot: reviewed d35ba44, no issues found; no unresolved review threads.
  • Reviewer focus: Pipenv stale-install remedy now built once (vendor/pypi_pipenv.rs, scan/hosted/python.rs).
  • Slack announcement not sent this run (Slack send tool unavailable); next run should retry.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief (head d35ba44)

What it does. Hosted scan and vendored pypi each built their own wording for the Pipenv stale-install remedy. This PR moves that wording into one core helper, pypi_pipenv::stale_install_detail. For valid names the vendored text is unchanged. Hosted keeps its warning code and lead sentence, and drops the --product <purl> placeholder, which could not be run as written. A name that isn't a valid distribution name now prints as <package> instead of being put into the shell command.

Risk: low. Only warning text changes. The hosted wording change is described in the PR and covered by a test.

Look here

Verified

  • cargo test -p socket-patch-core --lib pipenv: 117 passed. cargo test -p socket-patch-cli --lib hosted::python: 5 passed.
  • Red check: with main's hosted code restored, pipenv_remedy_is_the_shared_core_wording fails, so the test exercises the change.
  • I compared the old and new code by hand. The vendored text is byte-identical for valid names.
  • CI is green on the head (ci-ok, clippy). Bugbot found nothing. CHANGELOG.md is untouched.

Changes I made: none.

Open questions (non-blocking)

  • The name guard accepts a name that starts with - (for example --pre). Requiring the first and last characters to be alphanumeric, as PEP 508 does, would close that.
  • pypi_reinstall.rs:89 still puts a raw name into commands without the guard. That was already true before this PR, and the PR calls it out of scope.
  • Hosted no longer mentions --product. docs/testing/pipenv-compatibility.md says Pipenv projects must pass --product explicitly, so it's worth confirming that dropping the hint is intended.

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


Generated by Claude Code

A decoded purl name such as `--pre` passed the remedy's name guard
and would be printed into `pip uninstall -y --pre`, where pip reads
it as an option. Names must now start and end with a letter or digit,
as PEP 508 requires; anything else prints as `<package>`.

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

Copy link
Copy Markdown
Collaborator Author

[agent] Thanks. On the open questions:

  • --led names: fixed in e7151f5. The guard now requires the first and last characters to be alphanumeric (PEP 508), and the core test covers --pre.
  • pypi_reinstall.rs:89: left out of scope here. It predates this PR, and its purl split is the A14 cleanup.
  • --product: dropping it is intended. --product names the project's product purl, and the old hosted text left a literal <purl> placeholder that a user could mistake for the package. Plain socket-patch vex matches the vendored text and CLI_CONTRACT.md, and vex itself says when a Pipfile-only project has to pass --product.

Generated by Claude Code

@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.

✅ 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 e7151f5. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. The Pipenv "unpatched release still installed" remedy was built twice, in hosted scan and in vendored pypi. The copies had drifted, and the hosted one told users to run socket-patch vex --product <purl> with the placeholder left in. Both now call one core helper, stale_install_detail, which sorts stale sites into own, shadowing and inherited copies and words each case once. Package names that aren't valid PEP 508 distribution names (shell metacharacters, option-like -x) print as <package> instead.

Risk: low. It changes advisory warning text only. Vendored output is byte-identical for valid names. The hosted text changes on purpose (the --product <purl> tail is gone) and keeps the same warning code; nothing in the crates matches on the old text.

Look here

  • pypi_pipenv.rs#L965: stale_install_detail and its name guard
  • pypi.rs#L809: vendored caller
  • hosted/python.rs#L155:`` hosted caller. The name comes from percent-decoded purl_parts, so the guard matters: `six%3Brm%20-rf` decodes to `six;rm -rf`.
  • Tests: pypi_pipenv.rs#L1098 and hosted/python.rs#L340.``` The hosted test fails on main because of the <purl> placeholder.

Verified

  • Compared the old and new vendored wording piece by piece: identical. Checked the guard against real PyPI names (uppercase, ., _, single-character), and all pass.
  • cargo test -p socket-patch-core --lib pipenv (117 passed), --lib stale (72 passed), cargo test -p socket-patch-cli --lib scan::hosted::python (5 passed). cargo fmt --check is clean.
  • CHANGELOG.md is untouched and there are no debug leftovers. CI on head: 31 passed, 19 skipped, 0 failed (ci-ok and clippy green). Bugbot: no issues.

Changes I made: none.

Open questions (non-blocking follow-up): pypi_reinstall.rs L89 still calls the public stale_install_remedy directly, without the name guard. That is not a regression, and the PR scopes it out. Moving the guard into stale_install_remedy itself would cover every caller.

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


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Ready for review confirmed at e7151f5 (the head changed after the earlier label comment).

  • CI: every check on e7151f5 is green (31 success, 19 skipped); mergeable.
  • Bugbot: reviewed e7151f5, no findings. No unresolved review threads.
  • Slack: not announced. The Slack send tool isn't available in this run.

Generated by Claude Code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[agent] Reviewed e7151f5887baff4b825f4d1d5e7b8ee94405bca9. Checked shared Pipenv remediation for own, inherited and shadowing installed copies. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.

@mikolalysenko
Mikola Lysenko (mikolalysenko) added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 1b529c8 Nov 2, 2026
50 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-pipenv-stale-remedy branch November 2, 2026 16:43
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

Development

Successfully merging this pull request may close these issues.

Build the Pipenv stale-install remedy in one core helper: hosted scan and vendored pypi each write it, and the copies drifted

3 participants