GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix JSON writers dropping CRLF/BOM/indent (#324, #351) by mikolalysenko · Pull Request #357 · SocketDev/socket-patch

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Sep 30, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Fixes #324
Fixes #351

Summary

npm locks and composer.json now keep their own layout when socket-patch edits them, and the undo commands put the original bytes back.

  • npm (npm lock rewrites drop CRLF (and hosted drops tab indent), so rollback and vendor --revert are not byte-exact; BOM locks are refused #324): four paths re-serialized package-lock.json / npm-shrinkwrap.json with LF endings: scan --mode hosted, its rollback, scan --mode vendored and vendor --revert. Hosted mode also forced a 2-space indent. They now render the lock through the existing vendor::common::JsonLayout, which keeps the BOM, indent, line ending and trailer.
    • A UTF-8 BOM lock, which npm installs from, used to be skipped as redirect_npm_lock_unparseable in hosted mode and refused as vendor_lockfile_version_unsupported in vendored mode. Both now read past the BOM and keep it.
    • VEX discovery (vex::discover::parse_json) and the npm lock inventory read BOM locks too, so a wired BOM lock is still attested.
  • Composer (Composer setup rewrites a CRLF composer.json as LF (and un-escapes \/ and \uXXXX), so setup --remove does not restore it byte-for-byte #351): setup / setup --remove no longer re-serialize the whole composer.json. A new splice_scripts edits only the top-level scripts value, the way Composer's own JsonManipulator does:
    • The value is rendered in the file's indent, line ending, and slash and unicode escaping. That rendering is render_in_style_of, factored out of the composer.lock entry splicer.
    • A new scripts is appended as the last member, and removing it cuts exactly that span back out. That's why setup + setup --remove is byte-exact.
    • When the root has no member to anchor on, it falls back to JsonLayout.
  • Docs: a CHANGELOG Fixed entry. README already promised a byte-for-byte vendor --revert / remove; this makes that true for CRLF, tab and BOM locks.

Root cause

The npm lock writers and Composer setup re-serialized the parsed document through either a fixed 2-space/LF serializer (patch/redirect/mod.rs serialize_json) or detect_indent + serialize_json. Neither keeps line endings or a BOM. serde_json::from_str / from_slice also reject a BOM outright. The layout-preserving JsonLayout already existed for package.json and yarn berry, but these writers never used it. The escape half of #351 needed a span-level edit, because any parse and re-serialize normalizes escapes.

Test evidence

Issue Case Regression test Red on main Green
#324 hosted rewrite: CRLF, tabs, BOM, BOM+CRLF+tabs (only resolved/integrity change) patch::redirect::tests::npm_lock_rewrite_keeps_crlf_tabs_and_bom ✅ fails ✅
#324 hosted rollback byte-exact: CRLF, tabs, BOM patch::redirect::takeover::tests::npm_revert_restores_crlf_tab_and_bom_locks_byte_for_byte ✅ fails ✅
#324 vendored rewrite keeps layout, BOM not refused, vendor --revert byte-exact vendor::npm_lock::tests::vendor_and_revert_keep_crlf_tab_and_bom_lock_layout ✅ fails ✅
#324 VEX discovers a wired BOM lock vex::discover::npm::tests::bom_prefixed_wired_lock_is_discovered ✅ fails ✅
#324 real npm 10.9.7: CRLF+BOM lock → vendor → fresh npm ci installs patched bytes → vendor --revert byte-exact e2e_vendor_npm_build::npm_vendor_keeps_a_crlf_bom_lock_and_reverts_it_byte_for_byte new ✅
#351 CRLF round trip setup::composer::tests::test_round_trip_preserves_crlf_line_endings ✅ fails ✅
#351 escaped-slash and escaped-unicode strings round trip test_round_trip_preserves_string_escapes ✅ fails ✅
#351 existing user scripts, CRLF + escapes, no trailing newline test_round_trip_with_user_scripts_keeps_crlf_and_escapes ✅ fails ✅

For the npm and VEX rows, red was checked by running the tests before the fix commit, and for VEX by reverting parse_json alone. The Composer tests ran red before splice_scripts was written.

  • SOCKET_PATCH_NPM_E2E_REQUIRED=1 cargo test -p socket-patch-cli --all-features --test e2e_vendor_npm_build --test e2e_redirect_npm_build -- --include-ignored: 14 + 15 passed (real npm 10.9.7, Node 22).
  • cargo test -p socket-patch-cli --all-features --test e2e_composer --test setup_matrix_composer -- --include-ignored: all passed.
  • e2e_vendor_composer_build: 15 passed, 2 failed. The 2 failures (composer_vendor_fast_path_heals_legacy_copy, composer_vendor_keeps_files_mirror_filters_would_drop) are sandbox-only. Composer here fell back to a source install, so the vendored copy carries .git/HEAD. That's the separate Composer vendor copies a --prefer-source package's .git into .socket/vendor, so git commits it as an embedded repo and a fresh clone installs an empty package #355, and this PR doesn't touch that code. Both pass in CI.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo fmt --all -- --check: the new code is rustfmt-clean. The remaining diffs are already on main, in files and hunks this PR doesn't touch.
  • cargo test --workspace --all-features --no-fail-fast: 10,400 passed and 22 failed. The 22 are the same sandbox-only set reported on Fix #258: state the real Maven Trusted Checksums floor #322, Fix Poetry venv discovery to match Poetry (#327, #329) #330, Fix npm VEX attesting packages with an unpatched bundled copy (#325) #337 and Fix npm rewiring git/URL/file lock entries (#326) #345: write-failure and permission tests that fail because the sandbox runs as root, plus the peak-RSS stage_local_artifact_caps_oversized_artifact_before_buffering.
  • The npm, PyPI and gem wrappers only dispatch, so they need no change.
  • CI on 0dbe6cc: green. 561 check runs: 559 success, 2 skipped (downgrade and canary). Bugbot's review of 0dbe6cc found no issues.

Notes

  • This touches rewrite_one_npm_lock and vendor/npm_lock.rs next to Fix npm rewiring git/URL/file lock entries (#326) #345, but at different lines: the parse and serialize calls here, entry selection there. Whichever lands second should need at most a trivial merge.
  • Not changed: the pnpm-path package.json writes and the Pipenv / composer.lock whole-document fallbacks still use detect_indent + serialize_json. No open issue reports them. They're candidates for the same JsonLayout switch in a follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GZ6Poya3NwsgeV8gN7WL3Q


Note

Medium Risk
Changes shared npm lock parse/serialize paths used by hosted scan, rollback, vendoring, and VEX; layout bugs could break installs or byte-exact revert, but behavior is narrowly scoped to formatting preservation with broad test coverage.

Overview
npm lock edits no longer normalize the whole file. Hosted redirect, hosted rollback, vendored vendor, and vendor --revert used to pretty-print package-lock.json / npm-shrinkwrap.json with LF (and hosted mode forced 2-space indent), so CRLF/tab/BOM locks got noisy diffs and undo did not restore original bytes.

Parsing now strips a leading UTF-8 BOM like npm (parse_json_text / parse_json_manifest), and writes go through JsonLayout via serialize_json_like so only resolved / integrity (and related) values change. VEX discovery and npm lock inventory accept BOM-prefixed locks instead of treating them as invalid JSON.

Regression coverage adds unit tests for CRLF/tabs/BOM shapes, golden hosted round-trips, vendored vendor/revert layout, BOM VEX discovery, and a real-npm e2e for CRLF+BOM vendor → npm ci → byte-exact revert.

Reviewed by Cursor Bugbot for commit a6b975b. Configure here.


Generated by Claude Code

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@claude
Start fix for #324, #351
3a0de20
Assisted-by: Claude Code:claude-opus-5-5
This was referenced Sep 30, 2026
npm lock rewrites drop CRLF (and hosted drops tab indent), so rollback and vendor --revert are not byte-exact; BOM locks are refused #324
Closed
Composer setup rewrites a CRLF composer.json as LF (and un-escapes \/ and \uXXXX), so setup --remove does not restore it byte-for-byte #351
Closed
Claude (claude) added 4 commits September 30, 2026 18:33
@claude
Add tests for JSON writers losing file layout
80389b3
Regression tests for #324 (npm lock CRLF, tab indent and BOM through
hosted rewrite, rollback, vendor and vendor --revert) and #351
(composer.json CRLF and string escapes through setup and
setup --remove). The composer tests pass with the splice-based
editor added in this commit; the npm ones fail until the next.

Assisted-by: Claude Code:claude-opus-5-5
@claude
Keep npm lock CRLF, tabs and BOM on rewrite
6a79bd2
The hosted and vendored npm lock rewriters, rollback and
vendor --revert now render the lock in its own layout (BOM, indent,
line ending, trailer) through JsonLayout, so a CRLF or tab-indented
lock gets a two-line diff and undo restores the original bytes.
A UTF-8 BOM lock, which npm installs from, is now read past its BOM
instead of being skipped (hosted) or refused with the wrong code
(vendored), and VEX discovery reads it too.

Fixes #324

Assisted-by: Claude Code:claude-opus-5-5
@claude
Note the layout-keeping JSON edits in CHANGELOG
08007a9
Assisted-by: Claude Code:claude-opus-5-5
@claude
Add real-npm e2e for a CRLF + BOM package-lock
0dbe6cc
Vendors a CRLF lock with a UTF-8 BOM, proves a fresh npm ci installs
the patched bytes from it, and that vendor --revert restores the
lock byte for byte (#324).

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review September 30, 2026 18:59
@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

cursor[bot] reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor[bot]
cursor Bot reviewed Sep 30, 2026
View reviewed changes

@cursor cursor Bot left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mikolalysenko Mikola Lysenko (mikolalysenko) mentioned this pull request Sep 30, 2026
Fix npm crawler missing relocated dependency stores (#359, #362) #365
Merged
Mikola Lysenko @claude
Merge main into fix-json-writers-keep-layout to keep #324 on v5
0644c00
The v5 consolidation (#277) removed `setup` (with its Composer hook) and
replaced the takeover-ledger npm revert with the upstream restorer in
patch/redirect/upstream/npm.rs, which still re-serialized the lock with
LF and a 2-space indent and choked on a BOM. Port the revert half of the
fix there (parse past the BOM, write in the lock's own layout) and pin it
with a CRLF/tab/BOM round-trip test in upstream_restore_golden. Drop the
#351 composer.json splice and its lock_text refactor, which only served
the removed `setup`; keep both CHANGELOG entries.

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

Mikola Lysenko (mikolalysenko) commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

cursor[bot] reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor[bot]
cursor Bot reviewed Oct 1, 2026
View reviewed changes

@cursor cursor Bot left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Mikola Lysenko (mikolalysenko) commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Ready for review on 0644c00: mergeable, 0 commits behind main. CI: 488/488 completed checks green (6 workflow skips, no failures). Bugbot reviewed 0644c00 and found no new issues; no unresolved review threads. Composer setup half was dropped in the v5 merge since v5 removed setup; npm takeover fix was ported to upstream/npm.rs.


Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Tanmay182003
Tanmay Singla (Tanmay182003) approved these changes Oct 1, 2026
View reviewed changes
@mikolalysenko Mikola Lysenko (mikolalysenko) mentioned this pull request Oct 1, 2026
Yarn classic hosted and vendored rewrites convert LF lines of a mixed CRLF/LF yarn.lock to CRLF, so rollback is not byte-exact #467
Closed
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 1, 2026
@claude
Merge origin/main into agent/fix-json-writers-keep-layout
a6b975b
Resolve the redirect/mod.rs import conflict by keeping both the
JsonLayout helpers this branch needs and main's npm_origin imports.

Co-Authored-By: Claude <[email protected]>
@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

cursor[bot] reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor[bot]
cursor Bot reviewed Oct 1, 2026
View reviewed changes

@cursor cursor Bot left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

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 a6b975b. Configure here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Mikola Lysenko (mikolalysenko) commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Ready for review on a6b975b: mergeable, 0 commits behind main.

  • Merge: main brought in. The one conflict was the import block in crates/socket-patch-core/src/patch/redirect/mod.rs; both sides were kept (vendor::common::{parse_json_text, JsonLayout} plus main's vendor::npm_origin imports).
  • CI: 484/484 check runs green (6 skipped). native (windows-latest, 1.1.0) failed once: its hosted cells got HTTP 503 from the patch host, and the same job passed on the previous head. One re-run of that job came back green.
  • Bugbot: reviewed a6b975b, no new issues, and no unresolved threads.
  • Reviewer note: the merge commit adds no logic. The earlier approval was given on 0644c00.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit c9485bc into main Oct 1, 2026
541 of 542 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-json-writers-keep-layout branch October 1, 2026 16:45
This was referenced Oct 3, 2026
Vendored pnpm rewrites a CRLF package.json as LF, and vendor --revert does not restore it #662
Closed
Walk package-lock entries once for inventory, vendored, hosted and restore #663
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cursor cursor[bot] cursor[bot] left review comments
@Tanmay182003 Tanmay Singla (Tanmay182003) Tanmay182003 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Composer setup rewrites a CRLF composer.json as LF (and un-escapes \/ and \uXXXX), so setup --remove does not restore it byte-for-byte npm lock rewrites drop CRLF (and hosted drops tab indent), so rollback and vendor --revert are not byte-exact; BOM locks are refused

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@mikolalysenko @Tanmay182003 @claude
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.