Skip to content

Fix pylock revert misplacing wheel hashes (#1623) - #1625

Open
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
agent/fix-pylock-restore-table-positions
Open

Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
agent/fix-pylock-restore-table-positions

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1623

Summary

vendor --revert / remove of the first-vendored of two packages in a
pip lock pylock.toml wrote an invalid lock with exit 0. idna's
[packages.wheels.hashes] table was printed after six's [[packages]]
header, so pip refused the file and six's revert reported drift. After
this change the revert gives back the original bytes, in either order.

Root cause

restore_document (crates/socket-patch-core/src/vendor/pypi_lock.rs)
merges the recorded original back into the live document by cloning
items out of the parsed original. Parsed standard tables keep their
position in the text they came from, and toml_edit prints tables in
position order. Once the live document numbers its tables differently
(another package's [[packages.wheels]] was replaced by an inline
archive, or the user added a package in front), a restored sub-table
could print after the next array element's header and re-parse as part
of that element. A single vendored package never hit this because
restore_document short-circuits when the live text equals the
vendored text.

Fix

  • restore_header_table: a table-valued key the revert re-adds (vendoring
    removed it, as pip lock's wheels is replaced by archive) goes back
    at its original key slot. Its copied sub-tables drop their recorded
    positions, so they print right after the table before them.
  • keep_under_owner: a final O(n) pass keeps every table under an
    array-of-tables element between that element's header and the next
    one. TOML reads meaning only from that order. The pass covers the
    replace paths too (for example a legacy uv [[distribution.wheel]]
    that vendoring changed rather than removed), and it leaves alone any
    table the live text already placed.
  • A key restored in place keeps its comment, and a table is no longer
    written into an inline table.

Tests (red → green)

Issue / case Test Without fix With fix
#1623 pip lock, two packages, revert first-vendored, then the other (both orders) pip_lock_wheel_tables_revert_under_their_own_package FAIL: hashes printed under six (the issue's exact bytes) pass, byte-identical
#1623 with a later attestation-identities sub-table, and with a package the user added in front pip_lock_wheel_tables_revert_before_later_sub_tables FAIL pass
#1623 replace path: [[distribution.wheel]] with a user package in front replaced_wheel_tables_stay_under_their_own_element FAIL (wheel printed under the user's package) pass
layout guards (whole-table restore, CRLF checkout, dotted keys) whole_table_restore_keeps_original_sub_table_order, crlf_checkout_restores_out_of_order_tables_in_place, re_added_sub_table_keeps_dotted_key_order pass on main pass

Ran locally:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • cargo test -p socket-patch-core --lib: 6540 passed. 4 failed, all because the sandbox runs as root, so chmod-based write-failure tests can't fail a write. None touch this code: 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. CI runs as non-root.
  • cargo test -p socket-patch-cli --all-features --test e2e_vendor_pypi_build (with --ignored too, real uv 0.11.32), mode_migration_pypi, in_process_vendor_pypi_takeover, vendor_ledger_schema_e2e: all pass.
  • I couldn't run the full cargo test --workspace --all-features: linking every test binary used up the sandbox's disk allowance, so I ran the crates and suites above instead.
  • Wrappers (npm/, pypi/, gem/) are untouched; this is core TOML merging only.

/code-review high (4 rounds)

I fixed the confirmed findings, including two earlier designs that the
review showed reordered unrelated pyproject sections or were cubic on
large locks. Not fixed, all layout-only (the lock parses to the same
data, never under the wrong parent):

  • A whole-table restore keeps the recorded positions, so a top-level
    table can move when the user added tables in front. Main does the
    same (pre-existing).
  • A re-added plain key (for example pylock index) is still appended
    at the end of its table. Main does the same (pre-existing; an
    existing test pins the append order).
  • A re-added sub-table whose previous sibling has a descendant printed
    out of order later in the file, and a clamped table inside an element
    the user both edited and shifted, can print in a different spot than
    the original. Neither shape comes from pip lock, uv or pdm
    writers.

Not addressed: the issue's side note about comment decor (Poetry
routine, ledger #309) beyond keeping the live key's own comment on an
in-place restore.

https://claude.ai/code/session_01QaZG4zyZ3anK6ChQi3KwCF


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Reverting one of several vendored packages in a `pip lock`
pylock.toml put that package's [packages.wheels.hashes] table after
the next [[packages]] header. pip then refused the lock and the other
package's revert reported drift, while the CLI exited 0.

The restore copied tables from the recorded original text together
with their positions in that text, and toml_edit prints tables in
position order. Copied tables now drop those positions, so they print
inside the table they are restored into, and a replaced key keeps its
comments.

Fixes #1623

Assisted-by: Claude Code:claude-opus-5-5
The first fix for #1623 dropped the recorded positions of restored
tables entirely. That put them at the end of their parent, so a
revert could print a package's wheels after its other sub-tables, or
move a pyproject [tool.uv.sources] above a [tool.ruff] it originally
followed. The file still parsed the same but was no longer the
original bytes.

Restored tables now keep their recorded position as a mark, and a
final pass places each one right after the live copy of the table it
followed in the original. That lookup stays inside the
array-of-tables element the restore paired, so a package the user
added since vendoring can't pull a restored table to the wrong
package.

Refs #1623

Assisted-by: Claude Code:claude-opus-5-5
Replace the cross-document placement pass with a local rule. Only
keys a revert puts back (vendoring removed them) drop their recorded
table positions; they go back at their original key slot and print
right after the sibling before them. Whole-table replacements keep
their recorded positions as before, so unrelated pyproject sections
keep their original order, and large locks don't pay a quadratic
lookup.

Refs #1623

Assisted-by: Claude Code:claude-opus-5-5
Restoring a vendored table that was changed rather than removed (a
legacy uv [[distribution.wheel]]) still copied its recorded
positions. A package the user added in front shifts the live
numbering, so the wheel printed under that package instead.

A final pass now keeps every table under an array-of-tables element
between that element's header and the next one. That is the only
order TOML reads meaning from, and the pass leaves tables the live
text placed alone. Re-added keys are put back by re-inserting the
entries instead of sorting, which left a dotted table's keys in the
parent's order, and an in-place restore no longer puts a table into
an inline table.

Refs #1623

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 October 17, 2026 10:34
@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 ea1498e. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent).

  • Head: ea1498ec
  • CI: 86/86 green (36 passed, 50 skipped), no main-wide failures
  • Bugbot: reviewed ea1498ec, no findings; no open review threads
  • Mergeable, no CHANGELOG.md change
  • Reviewer note: the change is confined to crates/socket-patch-core/src/vendor/pypi_lock.rs; look at keep_under_owner, the final pass that keeps each sub-table under its [[packages]] element after a revert.

Slack announcement could not be sent this run (no Slack send tool available); the next run will retry.


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