Skip to content

Fix pip reinstall advisory after unwind (#1953) - #1954

Merged
Mikola Lysenko (mikolalysenko) merged 5 commits into
mainfrom
agent/fix-pip-reinstall-advisory
Nov 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 5 commits into
mainfrom
agent/fix-pip-reinstall-advisory

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

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

Fixes #1953

Summary

After a hosted rollback, a vendored rollback or vendor --revert of a requirements.txt pin, a plain pip install -r reports Requirement already satisfied and keeps the patched wheel. pip freeze also keeps naming the patch server, or the vendored wheel that vendor --revert just deleted. Before this PR the CLI told pip users the opposite: rollback printed the generic "until the next package-manager install" note, and vendor --revert printed no advisory. pip users now get the same *_pypi_reinstall_required advisory that PDM, uv and Pipenv already get (#477 / #1308).

Root cause

commands/pypi_reinstall.rs names the reinstall that brings back the upstream build after an unwind, but its Tool enum only knew PDM, uv and Pipenv. Tool::of_flavor("requirements") and Tool::of_file("requirements.txt") returned None, and remedy had no pip arm. Hosted rollback, vendored rollback, remove, vendor --revert and scan --prune all route through that one helper, so the fix belongs there.

Change

  • Tool::Pip: the vendored requirements flavor maps to it. A hosted pin maps to it when core's hosted restore reads the wiring file as a requirements file. The new upstream::is_requirements_file reuses format_of, so the CLI and core can't disagree about which files count.
  • Tool::of_files: when a hosted pin is wired by both a lock and a requirements file (a uv export requirements.txt next to uv.lock), the lock tool's remedy wins, because that is the sync that keeps the patched build.
  • pip remedy: pip uninstall -y <name>, then pip install -r <file> for each requirements file that wired the pin (with the uv pip pair as an alternative). Hosted reads the files from the restored pin and vendored from the entry's wiring records; lock files are filtered out, paths are shell-quoted, and an entry with no recorded wiring gets a <requirements file> placeholder instead of a guessed name. The uninstall-then-reinstall form keeps the requirements file's own version range, --index-url and --hash options. A pinned --force-reinstall name==version would drop them.
  • Rollback's note qualifier now reads "(pip, PDM, uv and Pipenv keep a same-version install through a plain install or sync: …)".
  • CLI_CONTRACT.md rows for reinstall_required, vendor_pypi_reinstall_required and redirect_pypi_reinstall_required are updated to match.
  • No wrapper changes (npm/, pypi/, gem/ only dispatch to the binary).

Out of scope

Test evidence

Issue Test Red on main → green here
#1953 hosted rollback / remove of a requirements.txt pin mode_migration_pypi::pypi_unwinds_name_the_reinstall_a_plain_sync_skips (new pip row) red: pip hosted ["rollback", "--yes"] names pip uninstall -y six`` panicked at mode_migration_pypi.rs:3798; green after the fix
#1953 vendored rollback, remove, vendor --revert, scan --prune same test, vendored loop and prune loop (pip row) green
#1953 classification and remedy text pypi_reinstall::tests::requirements_wiring_is_pip, advisory_names_pips_uninstall_and_reinstall new (they don't compile without Tool::Pip)
rollback note wording rollback::tests::reinstall_note_defers_to_the_pypi_advisory updated

Local runs:

  • cargo fmt --all -- --check: clean.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-cli --all-features --test mode_migration_pypi: 76 passed.
  • cargo test --workspace --all-features --no-fail-fast (on 194940d): 236 test binaries passed, 4 had failures (11 tests). The only failures are tests that can't pass in this sandbox, and none of them involves this change:
    • It runs as root, and root ignores the read-only dirs these tests rely on. Four core lib tests (copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_…, pypi_requirements::wire_failure_rolls_back_…) fail identically on unmodified main, as recorded in Build: share one test env guard in core (#1889) #1891. Three in_process_redirect tests (partial_lockfile_write_failure_…, redirect_json_mode_write_failures_…, vlt::…invalidation_failure_warns) and two repair tests (…cleanup_failure_…, …lock_file_unremovable) are write-failure tests of the same kind.
    • Two e2e_bun_lockb tests reach the live blob API from this sandbox (the missing_blob rollback). That is the issue ci-janitor Fix bun.lockb nightly e2e reaching the live blob API #1905 fixes.

Bugbot

One finding on c8cd874: the remedy hard-coded requirements.txt. Fixed in b572ade (pip_remedy_names_the_wiring_requirements_files), the thread is answered and resolved, and Bugbot was re-triggered.

/code-review high findings

I verified all 7 and fixed them in 194940d: lock tool beats a requirements file; one requirements-file classifier shared with core; uninstall plus pip install -r instead of a pinned force-reinstall (this also covers a missing version, version ranges, index and hash options, and the percent-encoded version); "install or sync" wording.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
After a hosted rollback, a vendored rollback or `vendor --revert` of a
requirements.txt pin, a plain `pip install -r` reports the requirement
already satisfied and keeps the patched wheel, while `pip freeze` keeps
naming the patch server or the deleted vendored wheel. The CLI only
named the reinstall for PDM, uv and Pipenv, so pip users got the
generic "until the next package-manager install" note (false for pip)
or, from `vendor --revert`, no advisory at all.

The reinstall advisory now knows pip: a vendored `requirements` entry
and a hosted pin in any requirements file (`*.txt`, `*.in`) name
`pip install --force-reinstall --no-deps <name>==<version>` and the
`uv pip` equivalent, and rollback's note lists pip with the tools a
plain install does not refresh.

Fixes #1953

Assisted-by: Claude Code:claude-opus-5-5
Review fixes for the pip reinstall advisory:

- A hosted pin wired in both uv.lock and a `uv export`
  requirements.txt kept getting pip's remedy instead of uv's. A lock
  tool now wins over a requirements file beside it.
- The CLI had its own requirements-file rule (`.in`, any case) that
  disagreed with the hosted restore. It now asks core's restore which
  files it reads as requirements.
- The remedy pinned `--force-reinstall name==version`, which ignores
  the file's index and `--hash` options and its version range. It now
  says to uninstall the package and reinstall from the requirements
  file.
- Rollback's note says "a plain install or sync", which fits both pip
  and the lock tools.

Refs #1953

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 November 1, 2026 21:27
@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.

The pip advisory always told users to run `pip install -r
requirements.txt`, even when the restored pin lived in another file
such as reqs/dev.txt, so following it did not bring the upstream
release back. The advisory now names the requirements files that
wired the pin: hosted takes them from the restored pin, vendored from
the entry's wiring records. An entry with no recorded wiring gets a
placeholder rather than a guessed file name.

Refs #1953

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.

✅ 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 b572ade. 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: labeled Ready for review.

  • Head: b572ade917811435a4421b67d945baeffa7ebfa6
  • CI: 31/31 required checks green (19 path-skipped)
  • Bugbot: reviewed b572ade, no new issues. Its one earlier finding (the pip remedy always named requirements.txt) was fixed in b572ade, and that thread is resolved.
  • For reviewers: the pip advisory now names the requirements files that wired the pin. See pypi_reinstall.rs and hosted_unwind.rs.
  • Slack announcement could not be sent in this run (no Slack send tool available); a later run will retry.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. After a rollback, remove, vendor --revert or scan --prune unwinds a requirements.txt pin, pip install -r says Requirement already satisfied and keeps the patched wheel. This PR adds a Tool::Pip arm to the shared pypi_reinstall helper, so pip users get the same *_pypi_reinstall_required advisory (pip uninstall -y <name>, then pip install -r <the wiring files>) that PDM, uv and Pipenv users already get.

Risk: low. It only changes the text of the advisory and rollback's note. No lock, wiring or install behavior changes. When a pin is wired by both a lock and a requirements file, the lock tool's remedy still wins, so existing PDM/uv/Pipenv output stays the same.

Look here

  • pypi_reinstall.rs:77: of_files picks the lock tool over a sibling requirements file (min_by_key on == Pip, otherwise the same first-match order as before)
  • pypi_reinstall.rs:150: the pip remedy text, with a <requirements file> placeholder when no wiring is recorded
  • pypi_reinstall.rs:116: shell_word quotes paths for a POSIX shell
  • hosted_unwind.rs:156 and vendored_backend/mod.rs:148:`` how the hosted and vendored paths pass their wiring files in
  • upstream/mod.rs:815:``` is_requirements_file reuses core's `format_of`, so the CLI and the hosted restore agree on which files are requirements files

Verified

Changes I made: none.

Open questions (not blocking)

  • The quoted paths are POSIX-only. On Windows cmd, a wiring path with a space would print as 'my reqs/base.txt', which cmd doesn't parse. It's advisory text only and rare.
  • pylock.toml and Hatch keep the generic note. The PR lists them as out of scope and suggests a follow-up issue.

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


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 b572ade917811435a4421b67d945baeffa7ebfa6. Checked pip reinstall advisories, requirements wiring paths, lock-tool precedence and rollback/remove/vendor propagation. 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 588711d Nov 2, 2026
50 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-pip-reinstall-advisory branch November 2, 2026 16:46
Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Nov 2, 2026
Resolve the conflict with #1954 in VendoredBackend::revert: the
entry's wiring files, read from the cloned entry before the revert
drops it, go to push_vendor_advisory as main now requires.

Assisted-by: Claude Code:claude-opus-5-5
Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Nov 2, 2026
Resolves hosted_unwind.rs against #1954's pip reinstall advisory:
main's Unwound/of_files version now sits inside the merged post-flush
block.

Assisted-by: Claude Code:claude-opus-5-5
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