Skip to content

Fix pnpm lock/workspace readers missing a BOM (#903, #904, #905) - #909

Open
Mikola Lysenko (mikolalysenko) wants to merge 4 commits into
mainfrom
agent/fix-pnpm-bom-readers
Open

Mikola Lysenko (mikolalysenko) wants to merge 4 commits into
mainfrom
agent/fix-pnpm-bom-readers

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #903
Fixes #904
Refs #905 (this PR does steps 1 and 2 of its proposed change; step 3, re-pointing the ~50 inline strip_prefix('\u{feff}') sites, stays open as the issue's follow-up slice)

Summary

Windows editors that save "UTF-8 with signature" put a BOM at the start of pnpm-lock.yaml / pnpm-workspace.yaml. pnpm reads those files normally, but socket-patch's pnpm readers matched column-0 text and missed the first line:

Root cause

formats::pnpm had no BOM handling. head_lock_version, lock_versions (behind lock_version_major and may_need_store_flag), is_pnpm_lock_text, unsupported_early_shrinkwrap and workspace::top_level_key all match a column-0 literal. The crate has no shared BOM helper (#905): four named copies plus ~50 inline strips, and each new reader decides for itself.

Changes

Per-issue test mapping (red on main, green here)

Issue Test Without fix
#903 formats::pnpm::tests::bom_lock_reads_like_its_plain_twin (9.0, 6.0, 5.4, 5.2 and shrinkwrap twins: is_pnpm_lock, sniff_lock_grammar, lock_version_major, may_need_store_flag, check_v9_lock_version, entries) FAILED
#903 vendor::pnpm_lock::tests::bom_lock_vendors_and_reverts_byte_exact FAILED: Refused vendor_lockfile_version_unsupported: … has no lockfileVersion in its head
#903, #904 CLI in_process_redirect_pnpm::hosted_bom_lock_and_workspace_read_like_their_plain_twins (scan creates the trust scaffold, rollback restores the BOM lock byte-exact, a BOM trustLockfile: false/true is kept and not duplicated) n/a (added after the fix; it covers the same readers)
#904 formats::pnpm::workspace::tests::top_level_key_skips_a_leading_bom FAILED
#904 hosted::guidance::tests::workspace_trust_plan_reads_a_bom_first_key FAILED
#904 vendor::pnpm_lock::tests::bom_workspace_override_inserted_beside_existing_not_duplicated FAILED
#905 formats::text::tests::*, hosted::governing_root::tests::workspace_lockfile_dir_reads_the_top_level_key (extended) refactor coverage

Verification

  • CI on 7515fa7: all 547 check runs completed (541 success, 6 skipped, none failed or pending). Bugbot's review of 7515fa7 found no new issues, and there are no review threads.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-core --all-features: the only local failures are 4 permission-denial tests that can't fail as uid 0. They fail identically on origin/main in this sandbox and pass in CI.
  • cargo test -p socket-patch-cli --all-features --lib and the in_process_redirect_pnpm, in_process_rollback_hosted, hosted_memory_{engine,parity,rollout}, e2e_safety_pnpm, e2e_vendor_pnpm_build and e2e_npm suites: all pass locally.
  • cargo fmt --all -- --check already fails on main with this toolchain, and CI doesn't gate on it. I formatted only the changed lines and left unrelated files alone.

Follow-ups

🤖 Generated with Claude Code

https://claude.ai/code/session_0167b49WbzczWBJxsnUhCNEy


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
A pnpm-lock.yaml or pnpm-workspace.yaml saved with a UTF-8 BOM (as
Windows editors do with "UTF-8 with signature") is read normally by
pnpm, but socket-patch's pnpm readers matched column-0 text and missed
the first line:

- hosted scan skipped the trustLockfile auto-config, so frozen pnpm
  11/12 installs failed; rollback, remove and list then refused the
  lock the scan had just pinned, and vendored mode refused it as
  having no lockfileVersion (#903)
- a BOM first key in pnpm-workspace.yaml was missed, so hosted and
  vendored appended a duplicate trustLockfile / overrides key that
  pnpm refuses to parse (#904)

Add formats::text with one split_bom/strip_bom pair, replace the four
named copies with it, and make the pnpm lock sniffs and the workspace
key reader skip one leading BOM. Splices keep the original line, so
the BOM stays byte-exact. lockfileDir is now read through the same
workspace key reader (#905).

Assisted-by: Claude Code:claude-opus-5-5
Covers the #903 and #904 user flows end to end: a BOM lock gets the
trustLockfile auto-config and rolls back byte-exact, and a BOM first
trustLockfile key in pnpm-workspace.yaml is respected instead of
duplicated.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 6, 2026 01:50
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

main has failed socket-patch-core's lib tests since Gradle support
(#646) and the digest helpers (#865) both landed. The guard test
production_digests_go_through_the_helpers flags three files #646 added
that still hash inline: crawlers/gradle_cache.rs, patch/jvm_jar.rs and
patch/sidecars/maven.rs. That breaks test, test-release and coverage on
every open PR.

Each inline sha1/sha256 call now goes through sha1_hex_of or
sha256_hex_of, which compute the same lowercase hex. Behaviour is
unchanged.

Assisted-by: Claude Code:claude-opus-5-5
(cherry picked from commit 659ac2c)

@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

Copy link
Copy Markdown
Collaborator Author

[agent] coverage failed on 7235dc3 in utils::digest::tests::production_digests_go_through_the_helpers, an architecture guard that is red on main too: three Gradle files from #646 hash inline, but #865's guard requires them to use the utils::digest helpers. That isn't this PR's change. The fix is #878, so I cherry-picked its commit (659ac2c → 7515fa7) here. It becomes a no-op once #878 lands. The guard test passes locally after the port.


Generated by Claude Code

@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.

✅ 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 7515fa7. Configure here.

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 6, 2026
Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 6, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Ready for review at head 7515fa7 (0 behind main).


Generated by Claude Code

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 6, 2026

This branch has not been deployed

No deployments
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