Skip to content

Fix vendored Poetry revert after a re-lock (#1615) - #1662

Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-poetry-vendored-revert-rerender
Open

Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-poetry-vendored-revert-rerender

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1615

Summary

After a poetry lock by a different Poetry version, a vendored Poetry patch could never be removed. vendor --revert drift-kept it (and still exited 0), rollback and remove exited 1, scan --mode hosted refused the takeover, and the "re-vendor" remedy did nothing. Now all four restore the registry entry and reclaim the vendored wheel, as long as the lock still carries exactly the wiring socket-patch wrote.

Root cause

revert_poetry (vendor/pypi_poetry.rs) undid the wiring only through revert_lock_fragment_splice_atomic, which needs the recorded vendored [[package]] text byte for byte. Each Poetry version renders that unit a little differently:

  • 2.x adds groups.
  • 1.4+ drops category.
  • 1.8 drops groups.
  • A lock-1.1 → 2.x re-lock moves [metadata.files] into the unit.

The vendored source and patched hash survive every one of these re-renders, but the byte match fails, so the revert read it as drift forever. The vendored → hosted takeover reverts through the same function, so it was stuck too.

Fix

revert_poetry still tries the verbatim splice first. When that can't place the unit, restore_re_rendered handles it on the parsed live lock:

  • Already restored: if nothing in the lock names the vendored wheel URL or its patched hash (an earlier revert, or a full re-lock that moved to another version), there is nothing left to undo. Success, no write.
  • Re-rendered: on a lock 2.x, if exactly one name@version unit has a file source at the recorded .socket/vendor/pypi/<uuid>/… URL and files equal to the recorded patched list, drop the source and put back the recorded original file list. That list comes from the unit for lock 2.x originals and from [metadata.files] for 1.0/1.1 originals ([] stays []). Every field the re-lock added or dropped is kept as rendered.
  • Everything else stays drift, as before:
    • a different patched hash, wheel, source type or extra file;
    • a source removed while the patched hash stays;
    • a legacy (1.x) live lock;
    • a lock-0 original;
    • a recorded original that had its own source;
    • a lock toml_edit doesn't render back byte for byte. The restore re-renders the whole lock, so this check makes sure no other unit's bytes change.

Tests

Each test below failed before the fix and passes with it. The red runs are at the end of this section.

Issue variant Test
Vendored by 1.8.5, re-locked by 2.5.1 (groups added, lock 2.0 → 2.1) pypi_poetry::tests::revert_restores_a_unit_another_poetry_version_re_rendered (case 1.8 -> 2.5, LF + CRLF, dry run, second revert stays quiet)
Vendored by 2.x, re-locked by 1.8.5 (groups dropped) same test, case 2.4 -> 1.8
1.3.2 → 1.8.5 within lock 2.0 (category dropped) same test, case 1.3 -> 1.8
Lock 1.1 (Poetry 1.1.15) re-locked into lock 2.x revert_restores_a_legacy_unit_re_locked_into_lock_2 (empty and listed [metadata.files])
vendor --revert / rollback / remove through the CLI mode_migration_pypi::poetry_unwinds_after_a_relock_by_another_poetry_version
Vendored → hosted takeover (issue comment) mode_migration_pypi::poetry_vendored_to_hosted_after_a_relock
Full re-lock onto another version revert_converges_after_a_relock_onto_another_version
Guards: edited wiring is still drift re_rendered_unit_with_edited_wiring_is_still_drift, re_rendered_restore_refuses_an_original_with_a_source

Commands run locally:

  • cargo fmt --all -- --check: clean.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-core --all-features --lib: 6594 passed, 4 failed. The 4 are permission 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_maps_error_and_leaves_lock_untouched, pypi_requirements::wire_failure_rolls_back_already_written_files). They fail only because this sandbox runs as root, which ignores chmod 0o555. None are in code this PR changes.
  • cargo test -p socket-patch-cli --all-features --test mode_migration_pypi --test in_process_redirect_poetry: 66 + 9 passed.
  • A full cargo test --workspace didn't finish locally because the sandbox ran out of disk; CI runs it.

Red runs, without the fix:

  • The two new core restore tests failed with vendor_lock_entry_drifted.
  • With pypi_poetry.rs reset to main, both new CLI tests failed.

Code review (/code-review high)

  • Fixed:
    • The restore copied a recorded source table into the wrong place (unreachable today); it now refuses such an original instead.
    • The "already restored" check accepted a unit that kept the patched hash.
    • A re-lock onto another version still drift-kept.
    • The whole-lock re-render now runs only on a lock toml_edit reproduces exactly.
    • The name lookup now reuses common::lock_units_named.
  • Not fixed:
    • The lock is read twice on the normal path. That's one extra small file read per Poetry revert; folding it into the shared splice would mean changing the shared signature that PDM also uses.
    • PDM uses the same byte-exact splice and likely has the same re-render problem. That's a separate backend and not reported yet; follow-up below.

Follow-ups

🤖 Generated with Claude Code

https://claude.ai/code/session_01W4vKB5zgAMhBnjCJU9vVv9


Note

Medium Risk
Changes Poetry vendored revert and hosted takeover paths that rewrite poetry.lock; behavior is heavily tested but lock parsing must stay conservative to avoid silent corruption.

Overview
Fixes #1615: vendored Poetry patches could not be removed after someone ran poetry lock with a different Poetry version, because revert relied on a byte-exact match of the recorded [[package]] fragment.

revert_poetry still uses the verbatim lock splice first; when that fails, restore_re_rendered parses lock 2.x, confirms the target unit still has the expected vendored file source and patched files, then drops the source and restores the recorded registry file list while keeping fields the re-lock added or removed (groups, category, lock version, etc.). Unchanged wiring edits, legacy locks, and tampered units still count as drift.

multiline_files in poetry_lock.rs is exposed for consistent files rendering on restore. New CLI and core tests cover re-lock scenarios (including vendored→hosted after re-lock).

Reviewed by Cursor Bugbot for commit 268bd9a. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
A poetry lock by another Poetry version re-renders the unit
socket-patch wired (2.x adds groups, 1.4+ drop category, a lock-1.1
re-lock moves [metadata.files] into the unit) but keeps the vendored
source and patched hash. vendor --revert, rollback and remove read
that as drift forever, so the patch could never be removed.

When the verbatim splice can't place the unit, restore it from the
live lock if it still carries exactly the wiring we wrote. Edited
wiring is still drift.

Fixes #1615

Assisted-by: Claude Code:claude-opus-5-5
A second revert after a re-lock restore must stay quiet, and a
lock-1.1 unit keeps its patched hash in [metadata.files], so read
the patched file list from there. An already restored unit counts as
done only when nothing in the lock still names the vendored wheel.

Assisted-by: Claude Code:claude-opus-5-5
vendor --revert, rollback, remove and the vendored -> hosted
takeover each restore a unit Poetry 1.8.5 re-rendered from a 2.4.1
lock, and reclaim the vendored artifact.

Assisted-by: Claude Code:claude-opus-5-5
Only call a unit restored when the lock names neither the vendored
wheel nor its patched hash, so a unit that lost its source but kept
the patched hash still counts as drift. Never restore a recorded
source, and only re-render a lock toml_edit reproduces byte for
byte, so no other unit's bytes change.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 22, 2026 05:10
@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 268bd9a. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Ready for review — burn-down agent.

  • Head: 268bd9a3ddcd60725d296133ab03c99c78504976
  • CI: 86/86 checks green (36 success, 50 skipped), no main-wide failures.
  • Mergeable against main, no conflicts.
  • Bugbot: reviewed 268bd9a — no findings.
  • Reviewer focus: restore_re_rendered in crates/socket-patch-core/src/vendor/pypi_poetry.rs re-renders the whole lock via toml_edit; check the byte-for-byte round-trip guard that keeps other units untouched.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. Fixes #1615: after a poetry lock by a different Poetry version re-renders the vendored [[package]] unit (adds/drops groups or category, or moves lock-1.1 [metadata.files] into the unit), vendor --revert, rollback, remove and the vendored→hosted takeover used to drift-keep the patch forever. revert_poetry now falls back to a structural restore on the parsed lock when the byte-exact splice can't place the unit, and treats a lock that no longer names the vendored wheel or its hash as already restored.

Risk: medium. It rewrites poetry.lock, but only on lock 2.x, only when toml_edit round-trips the whole file byte for byte, only when exactly one name@version unit still has the recorded file source URL and the recorded patched file list, and only when the wiring is lock-only. Anything else falls through to the old splice and stays drift. The verbatim-splice path is unchanged.

Look here

  • pypi_poetry.rs:448: the fallback in revert_poetry; dry run doesn't write, and a byte-identical result isn't rewritten.
  • pypi_poetry.rs:559: the "already restored" shortcut. It returns success and lets the artifact be reclaimed when neither the vendored URL nor any patched hash appears in the lock (including when the dependency was removed or re-locked onto another version).
  • pypi_poetry.rs:584: the byte-for-byte round-trip guard that keeps other units untouched.
  • pypi_poetry.rs:614: the actual restore (drop source, put back the recorded original files).
  • mode_migration_pypi.rs:545: CLI tests for revert/rollback/remove and the hosted takeover after a re-lock.

Verified

  • Read the full diff (3 files, +539/−1): core logic, the multiline_files visibility change, unit and CLI tests. CHANGELOG.md untouched; no debug leftovers or unrelated edits.
  • The guards match the description: foreign or pyproject wiring records, a non-file source, a changed URL or hash, an extra file, a legacy (1.x) live lock, a lock-0 original and an original with its own source all return None and keep the old drift behavior. Tests cover the edited-wiring and original-with-source refusals.
  • CRLF is preserved (preserve_line_endings) and tested in both endings. The paths compared are the recorded lock strings, so Windows separators aren't an issue.
  • The tests would fail without the fix: the author reports the red runs, and each new test asserts the restored lock text, not just exit codes.
  • Ran cargo test -p socket-patch-core --all-features --lib pypi_poetry locally on the head: 32 passed, 1 failed. The failure is wire_write_failure_maps_error_and_leaves_lock_untouched, a chmod test that can't pass when run as root (the sandbox runs as root). The PR doesn't touch it, the author listed it, and it passes in CI. All 5 new core tests pass.
  • CI 86/86 green on 268bd9a (ci-ok and clippy pass). Bugbot reviewed 268bd9a and found nothing. No open review threads.

Changes I made: none.

Open questions: none blocking. Follow-up already noted in the PR: PDM's vendored revert uses the same byte-exact splice and likely has the same re-render problem.

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


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