Skip to content

Fix mixed line endings lost in vendored requirements (#1567) - #1612

Open
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
agent/fix-requirements-mixed-eol
Open

Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
agent/fix-requirements-mixed-eol

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1567

Summary

A vendored rewrite of a requirements.txt (or a -r include) that
mixes CRLF and LF lines no longer re-terminates lines it never touched.
vendor --revert and remove now restore the file byte for byte.

Root cause

The three vendored requirements writers in vendor/pypi_requirements.rs
(plan_requirements, plan_rewire for the superseding re-vendor, and
revert_requirements) split the file with str::lines(), which drops
every \r. They then re-joined it with a single terminator, the file's
majority ending. So every minority-style line changed on vendor and again
on revert, while both reported success.

Fix

  • The writers now edit physical lines that keep their own terminator
    (Line { text, eol }), the way the hosted rewriter
    (patch/redirect/requirements.rs) already does. A vendor line ends
    the way the pin it replaces ended, and every other line is written back
    unchanged.
  • A replaced span (a hash-pinned \-continuation) records each
    earlier line's ending when it differs from the span's last line. Then
    even a continuation that mixes endings internally reverts exactly. A
    uniform span records plain texts as before, so ledgers for ordinary
    files don't change, and ledgers written before this PR still revert.
  • The ending-preserving splitter moved to utils::line_endings::split_lines,
    shared with the vendored Cargo.toml writer (vendor/cargo_manifest.rs),
    which had its own copy.

Tests (per issue)

  • Vendored requirements.txt rewrite re-joins every line of a mixed CRLF/LF file with one terminator, so untouched lines change and vendor --revert / remove can't restore the file #1567: mixed_line_endings_survive_vendor_and_revert covers the LF-majority
    and CRLF-majority repros from the issue, plus a CRLF pin before an
    unterminated last line. mixed_line_endings_around_a_continuation_span,
    a_continuation_span_with_mixed_endings_reverts_byte_identically,
    mixed_line_endings_survive_rewire_and_revert and
    mixed_line_endings_survive_a_transitive_append cover the other
    writers, and a_uniform_span_records_plain_texts covers the
    unchanged ledger.
  • Red → green: on main's writer, the first three new tests failed with
    exactly the issue's symptom, for example
    left: "idna==3.7\n./…whl # socket-patch vendor: six==1.16.0\nrequests==2.31.0\n# tail\n"
    against the expected "idna==3.7\r\n…\nrequests==2.31.0\r\n# tail\n".
    They pass with this change.

Verification run locally

  • cargo fmt --all -- --check: pass
  • cargo clippy --workspace --all-features -- -D warnings: pass
  • cargo test -p socket-patch-core --all-features: 6540 passed. 4 failed,
    and the same 4 fail on main: they are read-only-directory tests that
    can't fail a write when the sandbox runs as root (wire_failure_rolls_back_already_written_files,
    wire_write_failure_maps_error_and_leaves_lock_untouched,
    relax_loop_must_not_traverse_symlinked_root,
    an_unremovable_hidden_lock_keeps_every_store_entry). CI runs as non-root.
  • cargo test -p socket-patch-cli --all-features --lib: 921 passed
  • e2e_vendor_pypi_build -- --include-ignored: 55 passed.
    in_process_vendor_pypi_takeover, mode_migration_pypi,
    scan_vendor_requirements_unwired and scan_requirements_lock_only
    all pass. e2e_vendor_cargo_build -- --include-ignored: 40 passed
    (shared splitter).
  • I didn't run the full cargo test --workspace locally: linking every
    CLI test binary filled the sandbox disk. CI covers it.
  • No wrapper changes (npm/, pypi/, gem/ don't touch this path).

/code-review high findings

Fixed: the mixed-ending continuation span, the duplicated splitter,
missing tests for the span and append shapes, and simplified restore
logic. Not fixed:

  • Reverting a transitive (Added) line in a root file that had no
    final newline leaves the separator wire added. This predates this PR
    and is unrelated to mixed endings. The existing test
    transitive_append_to_root_without_trailing_newline documents it. A
    fix needs a new record shape for Added, which other readers
    (lock_inventory/recover.rs) key on, so it belongs in its own change.
  • Borrowing line text instead of cloning: requirements files are small,
    and the extra copy isn't measurable next to the I/O.

🤖 Generated with Claude Code


Note

Medium Risk
Changes vendored requirements file rewrite/revert behavior on disk; incorrect edge-case handling could corrupt user requirements files, though coverage is extensive and backward-compatible ledger revert is preserved.

Overview
Fixes #1567 by changing vendored PyPI requirements.txt wiring so mixed CRLF/LF files stay byte-identical on lines that are not edited.

Previously, plan_requirements, plan_rewire, and revert_requirements used str::lines() and re-joined with a single file-wide terminator, which re-terminated minority-style lines on both vendor and revert. They now edit physical lines (Line { text, eol }), join with per-line endings, and record span metadata so hash-pinned \-continuations with mixed internal endings revert exactly; older ledgers without embedded endings still revert using the vendor line’s terminator.

utils::line_endings::split_lines is added and cargo_manifest.rs drops its duplicate local splitter in favor of the shared helper.

CI pins uv to 0.13.1 in ci.yml and pipenv-compatibility.yml because 0.13.2 rejects Pipenv pylocks with environments = [].

New unit tests cover mixed endings across vendor, revert, rewire, transitive append, and continuation spans.

Reviewed by Cursor Bugbot for commit 9f0bad3. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
A vendored requirements.txt rewrite split the file with str::lines()
and re-joined every line with one terminator. In a file that mixes
CRLF and LF lines, every minority-style line was re-terminated on
vendor and again on revert, so `vendor --revert` and `remove` reported
success but left a file that differs from the original.

Vendor, the superseding re-vendor and revert now edit physical lines
that keep their own terminator, as the hosted rewriter already does.
A vendor line ends the way the pin it replaces did, and every other
line is written back byte for byte.

Fixes #1567

Assisted-by: Claude Code:claude-opus-5-5
Review follow-up. A hash-pinned requirement that spans several lines
with different endings now records each earlier line's ending when it
differs from the last line's, so the revert restores the span byte for
byte. Ledgers written before this change still revert as before.

The ending-preserving line splitter now lives in utils::line_endings,
shared with the vendored Cargo.toml writer instead of a second copy.

Refs #1567

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 16, 2026 17:14
@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 Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 16, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review at head a8e03aa.

  • CI: 36/36 checks green (50 skipped). No main-wide failures.
  • Mergeable against main, no conflicts. Only Rust sources change (line_endings.rs, cargo_manifest.rs, pypi_requirements.rs); CHANGELOG.md is untouched.
  • Bugbot: reviewed a8e03aa, no findings. No open review threads.
  • What to look at: how the continuation-pin path in pypi_requirements.rs keeps each line's own ending when it rewrites vendored requirements.
  • Slack announcement could not be sent this run (the Slack send tool is not available); the next run will retry.

Generated by Claude Code

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 20, 2026
uv 0.13.2 refuses the `environments = []` pylock that Pipenv 2026.8.0
writes, and the unpinned setup-uv steps picked it up. That turned the
pipenv:: e2e leg red on main's nightly and in the merge queue. Same
change as #1639; it no-ops once that lands.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01EjY8AbwdkRcREjHqS6jcFx
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

The merge queue dropped this PR because e2e (ubuntu-latest, e2e_vex_build, pipenv:: --ignored, 2026.8.0) failed. That failure isn't from this PR: uv pip install -r pylock.toml fails with "The target environment is not supported by the lockfile". The cause is uv 0.13.2+ rejecting the environments = [] that Pipenv writes, which the unpinned setup-uv step picked up. The same leg fails on main's nightly and on queue runs that don't include this PR.

I ported #1639's fix, which pins setup-uv to uv 0.13.1, in 9f0bad3, after merging main in. The pin has no effect once #1639 lands. This PR needs to be re-queued once CI is green.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Wenxin Jiang (@Wenxin-Jiang) this changed after your approval (on a8e03aae), so it needs another look before it goes through the merge queue.

Besides merges from main, the only new commit is 9f0bad3: the CI-only uv pin from #1639, byte-for-byte the same 12 added lines (version: "0.13.1" on the two setup-uv steps in ci.yml and pipenv-compatibility.yml). No crate code changed. It's there because uv 0.13.2 fails the pipenv:: e2e leg on every merge-queue entry. Once #1639 lands it does nothing.

If that's fine with you, re-approving 9f0bad33 is all this needs.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


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 9f0bad3. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review re-confirmed at the new head (burn-down agent).

  • Head: 9f0bad3
  • CI: 55 (36 success, 19 skipped) green. No main-wide failures.
  • Bugbot: reviewed 9f0bad3, no findings; no unresolved review threads.
  • Mergeable: clean. The head changed after the earlier approval (main merges plus the setup-uv pin port), so it needs a fresh approval before re-queueing.
  • Slack: not announced this run (send tool unavailable); 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

3 participants