[Mikola Lysenko (mikolalysenko)]

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

Fixes #1844

Root cause

utils/pdm_lock.rs and utils/poetry_lock.rs each had their own copy of the code that runs a lock rewrite around utils::lock_fragments:

• the one-dep rewrite (rewrite_*_lock_in);
• the batch rewrite (rewrite_*_lock_batch);
• the step-by-step fallback (rewrite_*_lock_steps).

Their mirrored equivalence tests were copied too. The two _steps loops had already drifted: PDM called rewrite.edits()? before its Unchanged check, and Poetry compared the text first. Inside lock_fragments, fragment pairing was also written three times (pair_fragments, finish, rewrite_batch), each with its own copy of the shape check.

Change

• One driver. lock_fragments now has a LockFormat trait. Each format supplies only:

  • WHAT, its name in error messages;
  • FRAGMENTS_IN, which reads a dep's fragments from the lock;
  • check, the dep's own refusals, settled before the lock is read;
  • plan, which reads the parsed lock and returns a refusal, not-found or a plan;
  • mutate, which applies the plan.

  rewrite_in, rewrite_all, rewrite_steps and rewrite_batch are written once against that trait. Pdm and Poetry implement it. Their batch/steps/closure glue is deleted.

• One pairing helper. changed_pairs checks the shape and pairs the fragments, and sole_in checks uniqueness. pair_fragments, finish and rewrite_batch all go through them. The batch keeps its one-pass overlapping sole_occurrences scan on purpose: it is conservative and only ever hands a case back to the step-by-step rewrite.

• Tests, one copy each.

  • The batch-vs-steps check is now one assert_batch_matches_steps::<F> in lock_fragments, which both formats' batch tests call.
  • The fixtures and dep mixes stay per format, because they are format data.
  • The shape-change test is kept once, in lock_fragments (it already ran for both format names). The two per-format copies are deleted.

• Public API unchanged. rewrite_{poetry,pdm}_lock{,_with_edits,_in,_all}, rewrite_poetry_lock_all_with_version and the *LockParse / *LockRewrite aliases all stay, because socket-patch-core is published. Each is now a few lines that forward to the shared driver. FragmentRewrite.name became an owned String, so a dep's lifetime no longer has to match the lock text's.

Behavior

No change to any rewritten lock's bytes, edits or refusals. The golden tests pdm_lock_reused_parse and poetry_lock_reused_parse and every exact-bytes test pass unchanged. Two internal differences:

• PDM steps loop: it now uses Poetry's order and compares the text before re-deriving edits. Before, a PDM re-run whose text was unchanged but whose edits failed to re-derive was Refused. It is now Unchanged, which is also what the batch path already reported. The issue notes this case looks unreachable, and no test or fixture reaches it.
• rewrite_poetry_lock_all_with_version: it now also returns the lock_version when it falls back to step-by-step. It is read inside Poetry::plan from a parse the rewrite takes anyway. The only caller, the hosted Poetry redirect, uses it to skip a re-parse. The batch test checks that it matches the output lock's version.

Review findings I didn't fix (from /code-review high)

• Read Poetry's lock_version in the redirect via parse_lock(original) instead of a OnceCell on the format: that would depend on the 2-slot parse memo still holding the original after the rewrite, and the hosted-scan parse-count tests pin those counts. The OnceCell is the narrower change.
• Keep per-format shape-change tests on real fixtures: Drive pdm.lock and poetry.lock rewrites through one lock_fragments driver instead of two mirrored triads #1844 asks for that test to be kept once, in lock_fragments. A per-format fragment-count regression is still caught by the exact-bytes, golden and batch-equivalence tests on every fixture.
• FragmentRewrite.name allocates: one small String per dep, next to a whole-lock render and re-parse. That isn't measurable, and it decouples the dep's lifetime from the text's.
• The "missing PDM package" fallback in rewrite_pdm_lock_in is dead today: it is kept on purpose, so that if PDM's plan ever returns None the single-dep API refuses instead of panicking.

Test evidence

• cargo fmt --all -- --check: clean.
• cargo clippy --workspace --all-features -- -D warnings: clean.
• cargo test -p socket-patch-core --all-features --lib: 6993 passed, 4 failed. The same 4 fail on unmodified main in this sandbox, because it runs as root and root can write to the read-only dirs those tests rely on (same list as Build: share one test RNG across randomized tests (#1868) #1882 and Build: share one test env guard in core (#1889) #1891): 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.
• cargo test -p socket-patch-core --all-features --test '*': all pass.
• cargo test -p socket-patch-cli --all-features --test e2e_vex_lockfile: 354 passed.
• cargo test -p socket-patch-cli --all-features --test e2e_vex_build -- --ignored poetry:: pdm:: with Poetry 2.4.3 and PDM 2.29.2: 6 passed.
• This is a refactor with no behavior change, so there is no red→green test. The existing golden, exact-bytes and batch-equivalence tests are the guard, and they pass unchanged.
• No wrapper changes. npm/, pypi/ and gem/ don't touch lock rewriting.

https://claude.ai/code/session_011f9aePxc3X2mz1fi6wUwYS

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 41bb169. Configure here.}

[Mikola Lysenko (mikolalysenko)]

[burn-down agent] Labeled Ready for review at 41bb169.

• CI: all PR checks green on 41bb169 (32 success, 50 skipped); mergeable, no conflicts.
• Bugbot: reviewed 41bb169, no findings; no unresolved review threads.
• Reviewer note: refactor only, in socket-patch-core/src/utils/{lock_fragments,pdm_lock,poetry_lock}.rs. The public per-format rewrite entry points are kept.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: Removes the duplicated PDM and Poetry lock-rewrite code. A LockFormat trait in lock_fragments.rs now has one shared one-dep, steps and batch driver, and fragment pairing is shared too (changed_pairs/sole_in). pdm_lock.rs and poetry_lock.rs keep their format-specific check/plan/mutate steps, and their public functions are now thin forwarders. Net change is +371/−358 across three files in socket-patch-core.

Risk: low–medium. This is the core lock-rewrite path, but it's behavior-preserving for every step and batch result. The only observable difference is noted below.

Look here:

• lock_fragments.rs:504: the shared steps loop decides Unchanged before calling edits(), which is the old Poetry order. PDM used to call edits() first. The results are the same, because unchanged text always gives edits() == Ok([]) and PDM plan never returns NotFound (pdm_lock.rs:195).
• lock_fragments.rs:576: in the batch, check now runs before plan
• lock_fragments.rs:211: changed_pairs and sole_in. Error messages and their order are unchanged.
• poetry_lock.rs:540 and :676: the lock version is now captured in a OnceCell during plan

Verified: Read the full diff, with an independent pass by a review subagent. cargo test -p socket-patch-core --lib lock gives 1833 passed. The 2 failures are root-only permission tests, and they fail the same way on main in this sandbox. The PR's CI run is green (ci-ok and clippy pass). Bugbot found no issues on this head. CHANGELOG.md is untouched. The two per-format shape-change tests that were deleted are still covered by pair_fragments_refuses_a_shape_change, which runs for both formats.

Changes I made: none.

Open questions (non-blocking): the version value returned by public rewrite_poetry_lock_all_with_version changed in two edge cases:

• When every dep fails check, it now returns None instead of Some("2.0").
• When the batch falls back to steps, it now returns Some(v) instead of None.

The doc comment matches the new behavior. The only caller (redirect/poetry.rs) reads the version only on Rewritten steps, so nothing in the CLI changes. Flagging it only because socket-patch-core is published.

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

---

Generated by Claude Code

[Wenxin Jiang (Wenxin-Jiang)]

[agent] Reviewed 41bb1690123ef6ee8adf674f1a1d1eddc270b443. Traced the shared LockFormat driver through PDM/Poetry planning, mutation, fallback, line-ending preservation and rollback fragment checks; inspected the batch/step equivalence coverage. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.