GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fix JSON writers dropping CRLF/BOM/indent (#324, #351) by mikolalysenko · Pull Request #357 · SocketDev/socket-patch Conversation
|
| 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.downgrade and canary). Bugbot's review of 0dbe6cc found no issues.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.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.
There was an error while loading. Please reload this page.
Assisted-by: Claude Code:claude-opus-5-5
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
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
Assisted-by: Claude Code:claude-opus-5-5
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
|
BugBot review Generated by Claude Code |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
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
|
bugbot run Generated by Claude Code |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
[burn-down agent] Ready for review on Generated by Claude Code |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
|
bugbot run Generated by Claude Code |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
[burn-down agent] Ready for review on
Generated by Claude Code |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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
There was an error while loading. Please reload this page.