Skip to content

Fix vendored Python reverts refusing sibling edits (#385, #474) - #481

Merged
Mikola Lysenko (mikolalysenko) merged 7 commits into
mainfrom
agent/fix-pypi-toml-merge-identity
Oct 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 7 commits into
mainfrom
agent/fix-pypi-toml-merge-identity

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #385
Fixes #474

Summary

Rolling back a vendored Python project no longer fails just because the user made an everyday edit next to the patched pin:

Root cause

Both reverts go through one three-way TOML merge, vendor::pypi_lock::restore_document (also used by the hosted Hatch helper in utils/hatch.rs). It:

  1. paired array elements by position and reported drift on any length change, so a sibling the user added, removed or moved blocked the revert; and
  2. ran the lock-package name/version identity check on every table, including pyproject [project], so a release bump counted as drift.

Fix

  • Arrays (of values and of tables): only the elements the vendoring changed (original[i] != new[i]) are looked up in the live array: first by exact text (ignoring the element's own whitespace/comments), then by identity (PEP 508 project name for requirement strings, canonical name + version for tables). Each one must map to exactly one live element, otherwise it is drift. Only those elements are restored, and the user's other elements are left as they are. A restored inline element keeps the live spacing/comments unless they are still the vendored ones.
  • The identity check now happens only when pairing array elements, not on keyed tables like [project].
  • Still drift (fail closed, live text kept): the vendored element was edited, re-resolved to another version, removed, renamed, or appears twice; or vendoring itself changed an array's length.

Tests (red → green)

Issue Test Without fix With fix
#385 vendor::pypi_hatch::tests::revert_keeps_unrelated_project_edits (version bump, comment on name, added dependency, added key) FAILED ok
#385 e2e e2e_vex_build hatch::hatch_project_dependency_vendored new step revert-after-project-edits (real Hatch 1.18.1: bump + idna==3.7, then vendor --revert) panicked: revert after project edits: exit Some(1) pass
#474 vendor::pypi_lock::tests::added_sibling_requirement_does_not_block_script_lock_revert FAILED ok
#474 e2e e2e_vendor_pypi_build vendored_uv_script_lock_manifestless_vex new step sibling-revert (real uv 0.8.17: uv add --script tool.py idna==3.7, vendor --revert, uv lock --script leaves the lock unchanged) panicked: an added sibling counted as drift restored, user addition kept
both sibling_strings_added_or_removed_around_the_vendored_element, re_resolved_package_with_added_sibling_is_drift, vendored_element_edited_or_removed_is_still_drift FAILED ok

Bugbot follow-up (1e6967d): a vendored element renamed past recognition plus a user-added twin of the original pin is drift, not "already restored" (already_restored_element_is_trusted_only_without_unknown_siblings, and a new case in vendored_element_edited_or_removed_is_still_drift; both red without the guard).

Existing drift tests (conflicting_fields_are_preserved_and_reported, reordered_packages_cannot_receive_another_packages_original, out-of-order and CRLF reverts, Hatch shared-permission rollback) still pass unchanged.

Commands run locally:

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test --workspace --all-features --no-fail-fast: everything passes except 12 tests that make a directory read-only with chmod to simulate write failures. This sandbox runs as root, which ignores chmod, so they fail identically on main (re-ran them against main's code). None of them involve the TOML merge.
  • SOCKET_PATCH_UV_E2E_REQUIRED=1 cargo test -p socket-patch-cli --all-features --test e2e_vendor_pypi_build -- --include-ignored: 20/20 ok (uv 0.8.17).
  • SOCKET_PATCH_HATCH_E2E_REQUIRED=1 SOCKET_PATCH_HATCH_E2E_VERSION=1.18.1 cargo test -p socket-patch-cli --all-features --test e2e_vex_build -- hatch:: --ignored: 4/4 ok.
  • cargo fmt --all -- --check is already red on main (CI doesn't run it). The changed files are formatted with the pinned 1.93.1 rustfmt, and the diff only touches the changed hunks.

No wrapper (npm/, pypi/, gem/) changes are needed: the merge is core-only.

Not in scope

#473 (hosted uv include-group restore in redirect/upstream/uv.rs), #379 and #382 are different code paths.

🤖 Generated with Claude Code


Note

Medium Risk
Changes core three-way TOML merge and revert success criteria for Python vendoring; incorrect pairing could restore wrong pins or refuse valid reverts, though ambiguous cases are designed to fail closed.

Overview
Vendored Python rollback no longer treats unrelated pyproject/lock edits as drift: restore_document in pypi_lock.rs now pairs only vendoring-changed array entries (by text or PEP 508 / name+version identity) instead of by index/length, so siblings like version bumps, new dependencies, or uv add --script additions are kept while the patched pin is restored.

Fail-closed guard: still_references_artifact blocks a “successful” revert when a user-added copy of the vendored requirement still points at .socket/vendor/pypi/{uuid}—wired into Hatch revert and lock revert warnings—so wheels are not dropped while still referenced.

Tests: unit cases for siblings, namesakes, and leftover artifact refs; Hatch e2e (revert-after-project-edits, revert-refuses-copied-reference); uv script e2e script_sibling_revert (#474).

Reviewed by Cursor Bugbot for commit f3f20c6. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Rolling back a vendored Hatch project failed with "pyproject.toml
changed since patching" after a routine release bump, a comment on
`name`, or a new dependency next to the patched one (#385). A vendored
PEP 723 script lock stayed vendored for good once `uv add --script`
added an unrelated requirement (#474).

Both reverts go through one three-way TOML merge. It paired array
elements by position, called any length change drift, and checked
lock-package name/version on every table, including [project].

The merge now restores only the elements vendoring changed, finding
each one in the live array by its text or its identity (PEP 508 name,
or table name + version). Siblings the user added, dropped or moved
are left alone. A vendored element that was edited, re-resolved,
removed or duplicated is still reported as drift.

Assisted-by: Claude Code:claude-opus-5-5
Adds end-to-end steps with the real tools: the vendored uv script
lane now runs `uv add --script tool.py idna==3.7` on a copy before
`vendor --revert` (#474), and the vendored Hatch project lane bumps
the release and adds a dependency before reverting (#385). Both
check the user's edits survive and the vendored wiring is gone.

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

Comment thread crates/socket-patch-core/src/vendor/pypi_lock.rs
Review found a gap in the new array merge: if the user renamed the
vendored element past recognition and also added a sibling equal to
the original pin, the revert paired the sibling as "already restored",
reported success, and deleted the vendored artifact the renamed line
still pointed at.

An "already restored" pairing now only counts when every other live
element is accounted for. Otherwise the revert reports drift and keeps
the artifact. Elements already back to their original text are also
skipped regardless of their spacing.

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.

Stale Bugbot comment from a previous run.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Ready for review on 1e6967d: CI is green (all 487 check runs completed with success or skipped), Bugbot's review of this head found no new issues, and its one earlier finding is fixed and resolved. Fixes #385 and #474, with red→green unit and real-tool e2e tests for each (see the table in the description).


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 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Reviewed 1e6967d91dcb3fc76bfb2ba0717b6b0a5ea6300a. Recommendation: changes needed.

P1 — Added sibling requirements can retain a reference to a wheel that revert deletes (pypi_lock.rs:494). unaccounted only blocks a pairing when already_original is true. When the recorded vendored row is still present, a second requirement using the same wheel URL with a different environment marker is preserved as a new sibling, while the recorded row is restored and the merge reports no drift.

Reproduced through the CLI: vendor a Hatch six==1.16.0 dependency, add a copy of the vendored requirement with ; python_version >= '3.8', then run vendor --revert. It returns exit 0 / success / removed: 1, leaves the added .socket/vendor/.../six-1.16.0-py3-none-any.whl requirement in pyproject.toml, and deletes that wheel directory. Subsequent installs now reference a missing file. Check residual artifact references before reporting a complete revert/deleting the artifact, or refuse the ambiguous added reference.

Validation: 21 vendor::pypi_lock and 5 vendor::pypi_hatch unit tests passed. An additional review-only CLI test for the added marked requirement failed with the behavior above. Full workspace and real package-manager matrices were not rerun.

Review found that a user-added copy of the vendored requirement (for
example with a `python_version` marker) survives the array merge as a
new sibling. The revert then reported success and deleted the wheel
that line still installs from.

Before a Hatch or Python-lock revert counts as complete, it now checks
that the restored file no longer mentions this patch's
`.socket/vendor/pypi/<uuid>` directory. If it does, Hatch fails and
the lock revert reports `vendor_lock_entry_drifted`, so the file and
the artifact are kept. Adds unit tests and a real-Hatch e2e step that
fail without the check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPpPuqqqgLkPXkcpsFfKFo
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Re the P1 in your review: confirmed and fixed in d1698d3, which also merges main.

Before a Hatch or Python-lock revert counts as complete, it now checks whether the restored file still mentions this patch's .socket/vendor/pypi/<uuid> directory when the original didn't. If it does, Hatch fails with "still references the vendored artifact" and the lock revert reports vendor_lock_entry_drifted. Either way the file and the wheel are kept. Wheels from other patches stacked on the same project are not affected.

Tests (each fails without the check):

  • pypi_hatch::revert_refuses_while_an_added_line_references_the_artifact: your repro, a copy of the vendored requirement with ; python_version >= '3.8'.
  • pypi_lock::leftover_artifact_reference_is_detected.
  • New real-Hatch 1.18.1 e2e step revert-refuses-copied-reference: vendor --revert exits non-zero and leaves pyproject.toml and the wheel. Without the check it exits 0, the behavior you saw.

Re-ran locally: vendor unit tests (2,132 pass; the 2 that fail are root-only chmod tests that also fail on main), uv e2e 20/20, Hatch e2e 4/4, and clippy.


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.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/vendor/pypi_lock.rs
Bugbot found that when the user removes the vendored array row, the
merge could still pair it by name with another row (a requirement's
identity ignores `specifier`). It then filled the original fields into
that row and reported a clean revert, rewriting the user's entry.

A table paired only by identity must now still hold at least one field
exactly as the vendoring wrote it, such as the vendored `path` or
`source`. Otherwise the revert reports drift and keeps the file and
the artifact.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPpPuqqqgLkPXkcpsFfKFo
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Follow-up review of f3f20c61540872cb5d3768b3e52797619b0022cc: ready from code review. The leftover-artifact check addresses my earlier finding, and I reviewed the additional safeguard against restoring a removed vendored row into a user-added namesake table.

Validation: 23 Python-lock tests passed at this exact head, including the new namesake-row regression and leftover-artifact check. The original independent Hatch CLI reproduction passed on the preceding commit d1698d36: vendor --revert exits 1, preserves both requirement rows, and keeps the referenced wheel (removed: 0). The final commit leaves that artifact-retention path unchanged.

No remaining blocking finding in the updated code. This supersedes my earlier changes-needed recommendation.

@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 f3f20c6. Configure here.

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator Author

[agent] CI: native (ubuntu-latest, 0.0.0-16) failed on f3f20c6. I don't think the failure comes from this PR:

  • It's the vlt (npm) backtest against production. Its log shows transport failure; retrying on several cases, and agent-direct ended with rollbackByteIdentical/rollbackOriginalBytes failing after those retries (31/34 as expected).
  • This PR only changes the Python TOML revert merge (vendor/pypi_lock.rs, vendor/pypi_hatch.rs), which vlt/npm never calls. The last commit's only change from d1698d3 is in pypi_lock.rs.
  • The same check passed on d1698d3 (job) and on main ffe6897 (job).

No fix to port. GitHub won't re-run the job while the rest of its workflow run is still going, so I'll re-run it once when the run finishes. If it fails again, I'll treat it as a real failure and investigate it.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] The re-run of native (ubuntu-latest, 0.0.0-16) passed, which fits the production transport failures in the first attempt. CI on f3f20c6 is now green (481 succeeded, 6 skipped). Bugbot is clean on this commit, and no review threads are open. The PR is waiting only on an approving review.


Generated by Claude Code

@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit 795a535 into main Oct 2, 2026
575 of 576 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-pypi-toml-merge-identity branch October 2, 2026 16:21
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Second pass reviewed f3f20c61540872cb5d3768b3e52797619b0022cc: ready from code review; no additional code change needed. The head is unchanged from my latest follow-up. Both the leftover-wheel-reference guard and the namesake-table guard remain in place, and I rechecked the current discussion against those fixes.

Prior validation remains applicable: 23 Python-lock tests passed at this exact head; the independent Hatch CLI reproduction passed at d1698d36, and its artifact-retention path is unchanged. No tests were repeated without a code change.

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