Fix PDM rollback leaving the patch after a re-scan (#331) - #375
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Conversation
Assisted-by: Claude Code:claude-opus-5-5
A hosted re-scan after `pdm add` or `pdm lock --update-reuse` records the still-patched pdm.lock unit as the edit's original, so rollback reports success but leaves the Socket url and patched hash in the lock (#331). These tests cover both PDM shapes (url kept, and url dropped with the patched hash kept) on LF and CRLF checkouts, and fail on main. Assisted-by: Claude Code:claude-opus-5-5
`pdm add` and `pdm lock --update-reuse` re-lay the redirected pdm.lock unit but keep Socket's url and patched hash, or just the hash on PDM 2.12-2.20. A hosted re-scan then adopted that still patched unit as the rollback target, so rollback reported success and deleted the ledger while the lock stayed patched (or, on 2.12-2.20, stopped installing). The rebase now adopts the re-scan's unit only once it no longer carries anything the redirect introduced; otherwise it keeps the recorded pristine unit in the relocked line endings. `pdm lock`, which re-resolves to the registry, behaves as before. Fixes #331 Assisted-by: Claude Code:claude-opus-5-5
Real `pdm add` moves the Socket url after `version` and re-lays `files` one entry per line; the regression test now reproduces that exact shape. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Rebase keeps stale fragment boundary
- Modified rebased_pdm_original to split fragments into unit and boundary, combining pristine unit with fresh boundary to handle cases where pdm add changes fragment boundaries.
Or push these changes by commenting:
@cursor push 51bb15195c
Preview (51bb15195c)
diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs
--- a/crates/socket-patch-cli/src/commands/scan/hosted.rs
+++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs
@@ -77,11 +77,61 @@
if !still_patched {
return fresh.original.clone();
}
- let lf = pristine.replace("\r\n", "\n");
+
+ // Split fragments into (unit, boundary). The boundary includes trailing
+ // blank lines, comments, and the next [[package]] header if present.
+ // PDM fragments include the package unit plus its boundary, so when the
+ // lock gains a new package that sorts after this one, the fresh boundary
+ // ends at that header instead of EOF.
+ let split_fragment = |s: &str| -> (&str, &str) {
+ let bytes = s.as_bytes();
+ let mut pos = bytes.len();
+
+ // Scan backwards to find the last non-boundary line
+ while pos > 0 {
+ // Find the start of the current line
+ let line_start = bytes[..pos]
+ .iter()
+ .rposition(|&b| b == b'\n')
+ .map(|i| i + 1)
+ .unwrap_or(0);
+
+ // Avoid infinite loop when line_start == pos (consecutive newlines)
+ if line_start == pos {
+ pos = line_start.saturating_sub(1);
+ continue;
+ }
+
+ let line = &s[line_start..pos];
+ let trimmed = line.trim_end_matches(['\r', '\n']).trim();
+
+ // Boundary lines are: blank, comments, or [[package]] headers
+ if trimmed.is_empty() || trimmed.starts_with('#') || trimmed.starts_with("[[") {
+ // Move to before the newline that precedes this line
+ pos = line_start.saturating_sub(1);
+ continue;
+ }
+
+ // Found a content line - boundary starts after it
+ return (&s[..pos], &s[pos..]);
+ }
+
+ // All lines were boundary (shouldn't happen)
+ (s, "")
+ };
+
+ let (pristine_unit, _) = split_fragment(&pristine);
+ let (_, fresh_boundary) = split_fragment(¤t);
+
+ // Combine pristine unit with fresh boundary, using fresh line endings
+ let lf_unit = pristine_unit.replace("\r\n", "\n");
+ let lf_boundary = fresh_boundary.replace("\r\n", "\n");
+ let combined = format!("{}{}", lf_unit, lf_boundary);
+
let relaid = if current.contains("\r\n") {
- lf.replace('\n', "\r\n")
+ combined.replace('\n', "\r\n")
} else {
- lf
+ combined
};
Some(serde_json::Value::String(relaid))
}You can send follow-ups to the cloud agent here.
A PDM lock fragment carries the unit's boundary: the blank lines and the next top-level header, or EOF. When `pdm add` appends a unit after the patched one, the kept pristine fragment still ended at EOF, so rollback replaced the live fragment with it and deleted the next unit's [[package]] header. The kept pristine body now takes the fresh fragment's boundary. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ 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 162a56a. Configure here.
|
[agent]
I haven't found a fix to port: the log shows no API or network error, only zero patches found on that one runner. I'll re-run the failed job once when the workflow run finishes. If it fails again, I'll treat it as real and dig into the captured Generated by Claude Code |
|
[burn-down agent] Closing as superseded by #277 (2463257). The bug in #331 was rollback replaying a stale ledger Generated by Claude Code |
2 participants
LLM Description written by Claude Code:claude-opus-5-5
Fixes #331
Summary
Hosted PDM
rollbacknow removes the patch afterpdm add <other>orpdm lock --update-reusefollowed by a re-scan. Before, rollback reportedsuccess, listed the purl as reverted and deleted the ledger whilepdm.lockstill carried the patch:urland patched sha256, so the project stayed patched with no way back.pdm syncrefused it.Root cause
When a hosted re-scan finds that a
pdm.lockunit has drifted from the ledger's recordednewfragment, the ledger merge rebases the edit instead of appending a chain. For PDM it always adopted the re-scan'soriginal. That's right afterpdm lock, which re-resolves to the registry and may reflow CRLF → LF. Butpdm addand--update-reusere-lay the unit while keeping the patch data. The re-scan'soriginalis then still patched, and it overwrote the pristine fragment.Fix
rebased_pdm_original(crates/socket-patch-cli/src/commands/scan/hosted.rs) is now used by both copies of the merge: the disk flow andhosted_memory/ledger.rs. It adopts the re-scan'soriginalonly when that fragment carries none of the strings the recorded edit introduced, i.e. none of the quoted values in itsnewthat itsoriginallacks: the hosted url and the patched hash.Otherwise it keeps the recorded pristine unit body, re-laid in the fresh fragment's line endings, and takes the fresh fragment's boundary (
split_pdm_boundary). A PDM unit fragment carries its trailing blank lines plus the next top-level header, or EOF, and the relock may have added a unit after this one.pdm lockbehaves exactly as before. Docs (docs/testing/pdm-compatibility.md) and CHANGELOG are updated.Test evidence
in_process_redirect_pdm::rerender_keeping_the_patch_then_rescan_rolls_back_to_the_registry. It runs scan →pdm addre-render → re-scan → rollback for {url kept, url dropped} × {no later unit, a laterzippunit appended} × {CRLF, LF} checkouts. It asserts that the ledger original stays pristine, rollback exits 0, and the lock equals the user's re-rendered lock with the upstream urllib3 unit back and every other unit (a later unit's header included) untouched.pdm add six==1.16.0on a Socket-rewritten lock movedurlto afterversion, re-laidfilesone entry per line, and kept the patched hash.keep_url=true crlf=true: the ledger's original must stay the pristine unit.later=true, which is Bugbot's finding: rollback dropped zipp's[[package]]header.relock_reflow_then_rescan_keeps_rollback_invertible(pdm lockCRLF→LF path).hosted_memory::ledger::tests::pdm_rebase_keeps_the_pristine_original_while_the_patch_survivescovers url+hash kept, hash only, and the clean relock.pdm_rebase_keeps_the_fresh_fragment_boundarycovers a boundary moved from EOF to a comment, a blank line and the next header.cargo clippy --workspace --all-features -- -D warnings: clean.cargo test --workspace --all-features --no-fail-fast(on 1afe13d): 10,395 passed, 20 failed. All 20 are write-failure tests that rely on read-only files or directories. This container runs as root (uid 0), so those writes succeed. For example,cli_setup_silent::setup_silent_keeps_apply_phase_error_outputfails identically onmainhere. None of them touches PDM or the ledger. CI runs them unprivileged.native (macos-latest, 1.2.23)failed once (every hosted cell reported 0 patches on that runner, details in the PR comment) and passed on its single re-run.cargo fmt --allisn't clean onmainwith the pinned 1.93.1 rustfmt (it would reformat 61 unrelated files), so I only formatted my own hunks.npm/,pypi/,gem/only dispatch to the binary).Per-issue checklist
pdm addorpdm lock --update-reuseand a re-scan #331 (PDM 2.26+/2.29pdm add/--update-reuse, url + patched hash kept):rerender_…withkeep_url=true, plus the unit-test case 1.pdm addorpdm lock --update-reuseand a re-scan #331 (PDM 2.12–2.20, url dropped, patched hash kept):rerender_…withkeep_url=false, plus the unit-test case 2.pdm addorpdm lock --update-reuseand a re-scan #331 (the added package sorts after the patched one):rerender_…withlater=true, pluspdm_rebase_keeps_the_fresh_fragment_boundary.pdm addorpdm lock --update-reuseand a re-scan #331 (control: plainpdm lockstill lands on the relocked registry lock): the existingrelock_reflow_then_rescan_keeps_rollback_invertible, plus the unit-test case 3.The first
rollbackright afterpdm add(before any re-scan) still fails closed with the drift error. The issue accepts that ("otherwise rollback should fail closed").🤖 Generated with Claude Code
https://claude.ai/code/session_018MTHxjeYFM5FWahRSVnaeH
Generated by Claude Code