[agent] Filed by the scheduled architecture audit routine (ecosystems and formats). Register: discussion #560 register.
Kind: bug (with a refactor fix). Source: new finding; review "CRLF/BOM/indent policy" (Part 4.4), register E64.
Problem
No module owns "a leading UTF-8 BOM is encoding, not content". Each reader decides for itself, on main @ 9c43dfc:
- Four named helpers do the same thing:
utils::serde::strip_bom, gradle::dsl::strip_bom, a private formats::yarn::strip_bom and cargo_manifest::split_bom.``
- About 50 more production sites in 30 files inline
strip_prefix('\u{feff}') or trim_start_matches('\u{feff}'), and several also re-add the BOM after an edit: split_bom, vendor::common, redirect/mod.rs, [`upstream/npm.rs`](https://github.com/SocketDev/socket-patch/blob/9c43dfc96a3da66ff83c0d3a977db9f496ecbd53/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs#L457-L459),`` plus requirements, Pipenv and Gradle.
- The copies have drifted.
vendor::common::parse_json_manifest strips exactly one BOM, which its test parse_json_manifest_reads_past_one_bom_only pins down. lock_inventory/pypi.rs,`` pypi_hatch.rs, `upstream/pypi.rs` and `vex/discover/pypi_other.rs` strip any number.
- The pnpm format module never strips it. Three
lockfileVersion readers in one file match a column-0 literal: head_lock_version (behind sniff_lock_grammar), lock_versions (behind lock_version_major and may_need_store_flag) and is_pnpm_lock_text. [`workspace::top_level_key`](https://github.com/SocketDev/socket-patch/blob/9c43dfc96a3da66ff83c0d3a977db9f496ecbd53/crates/socket-patch-core/src/formats/pnpm/workspace.rs#L20-L25``) doesn't strip it either, while the other pnpm-workspace.yaml key reader, governing_root::workspace_lockfile_dir,`` does. The yarn sniff is_berry_lock skips it on purpose.
Proof by execution. I ran a throwaway unit test twice on 9c43dfc with identical results. It used one pnpm 9 lock with one left-pad@1.3.0 entry, once plain and once with a \u{feff} prefix:
| reader (consumer) |
plain |
BOM |
PnpmLock::entries (inventory, hosted rewrite) |
1 entry |
1 entry |
inventory_project_diagnosed |
1 entry |
1 entry |
PnpmLock::is_pnpm_lock (VEX discovery) |
true |
false: "not a pnpm lockfile" diag |
sniff_lock_grammar / detect_npm_lock_flavor (vendored router) |
V9 / Pnpm |
Err vendor_lockfile_version_unsupported: "has no lockfileVersion in its head … re-lock with pnpm >= 9" |
lock_version_major (hosted trust gate) |
9 |
None |
workspace::top_level_key("trustLockfile: false") |
trustLockfile |
\u{feff}trustLockfile |
workspace_lockfile_dir("lockfileDir: ../x") |
../x |
../x |
formats::yarn::is_berry_lock (control) |
true |
true |
So a single BOM lock gets four answers inside formats::pnpm: it is readable, not a pnpm lock, unversioned, and unsupported. pnpm itself reads it (see #903).
Symptoms
Impact: medium. Each new reader repeats the decision, and every one that forgets it is a new bug. Three bug-hunt issues have hit this in different ecosystems so far.
Proposed change
- Add
formats::text with split_bom(&str) -> (&str, &str) (one BOM, as parse_json_manifest pins down) and strip_bom. Delete utils::serde::strip_bom, gradle::dsl::strip_bom, formats::yarn::strip_bom and cargo_manifest::split_bom, and re-point their callers.
- In
formats::pnpm: have head_lock_version, lock_versions, is_pnpm_lock_text and may_need_store_flag read strip_bom(text). Have the pnpm-workspace.yaml splices call top_level_key on a BOM-stripped first line, re-adding the BOM on write. Then make governing_root::workspace_lockfile_dir a top_level_key caller, which deletes its private key-prefix grammar.
- Re-point the inline
strip_prefix('\u{feff}') / trim_start_matches('\u{feff}') sites to the helper, file by file. Any site that keeps "any number of BOMs" must justify it in a comment.
Size and scope
- Steps 1 and 2: about 120 production lines in
formats/, utils/serde.rs, gradle/dsl.rs, vendor/cargo_manifest.rs and hosted/governing_root.rs.
- Step 3 is mechanical and can be a second PR (about 50 sites, net negative).
- Out of scope:
Acceptance criteria
Dependencies
[agent] Filed by the scheduled architecture audit routine (ecosystems and formats). Register: discussion #560 register.
Kind: bug (with a refactor fix). Source: new finding; review "CRLF/BOM/indent policy" (Part 4.4), register E64.
Problem
No module owns "a leading UTF-8 BOM is encoding, not content". Each reader decides for itself, on
main@9c43dfc:utils::serde::strip_bom,gradle::dsl::strip_bom, a privateformats::yarn::strip_bomandcargo_manifest::split_bom.``strip_prefix('\u{feff}')ortrim_start_matches('\u{feff}'), and several also re-add the BOM after an edit:split_bom,vendor::common,redirect/mod.rs,[`upstream/npm.rs`](https://github.com/SocketDev/socket-patch/blob/9c43dfc96a3da66ff83c0d3a977db9f496ecbd53/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs#L457-L459),`` plus requirements, Pipenv and Gradle.vendor::common::parse_json_manifeststrips exactly one BOM, which its testparse_json_manifest_reads_past_one_bom_onlypins down.lock_inventory/pypi.rs,``pypi_hatch.rs, `upstream/pypi.rs` and `vex/discover/pypi_other.rs` strip any number.lockfileVersionreaders in one file match a column-0 literal:head_lock_version(behindsniff_lock_grammar),lock_versions(behindlock_version_majorandmay_need_store_flag) andis_pnpm_lock_text.[`workspace::top_level_key`](https://github.com/SocketDev/socket-patch/blob/9c43dfc96a3da66ff83c0d3a977db9f496ecbd53/crates/socket-patch-core/src/formats/pnpm/workspace.rs#L20-L25``) doesn't strip it either, while the otherpnpm-workspace.yamlkey reader,governing_root::workspace_lockfile_dir,`` does. The yarn sniffis_berry_lockskips it on purpose.Proof by execution. I ran a throwaway unit test twice on
9c43dfcwith identical results. It used one pnpm 9 lock with oneleft-pad@1.3.0entry, once plain and once with a\u{feff}prefix:PnpmLock::entries(inventory, hosted rewrite)inventory_project_diagnosedPnpmLock::is_pnpm_lock(VEX discovery)sniff_lock_grammar/detect_npm_lock_flavor(vendored router)Pnpmvendor_lockfile_version_unsupported: "has no lockfileVersion in its head … re-lock with pnpm >= 9"lock_version_major(hosted trust gate)workspace::top_level_key("trustLockfile: false")trustLockfile\u{feff}trustLockfileworkspace_lockfile_dir("lockfileDir: ../x")../x../xformats::yarn::is_berry_lock(control)So a single BOM lock gets four answers inside
formats::pnpm: it is readable, not a pnpm lock, unversioned, and unsupported. pnpm itself reads it (see #903).Symptoms
trustLockfile: trueauto-config when pnpm-lock.yaml starts with a UTF-8 BOM, so pnpm 11/12 frozen installs fail with ERR_PNPM_TARBALL_URL_MISMATCH after a successful scan #903: the hosted trust gate misses a BOMpnpm-lock.yaml, and the frozen install fails.trustLockfile/overrides, so every pnpm install fails with "duplicate mapping key" after a successful scan #904:top_level_keymisses a BOM workspace key, and a duplicatetrustLockfile/overridesis appended.serde_json::from_strrefuses a BOMpackages.lock.json.Impact: medium. Each new reader repeats the decision, and every one that forgets it is a new bug. Three bug-hunt issues have hit this in different ecosystems so far.
Proposed change
formats::textwithsplit_bom(&str) -> (&str, &str)(one BOM, asparse_json_manifestpins down) andstrip_bom. Deleteutils::serde::strip_bom,gradle::dsl::strip_bom,formats::yarn::strip_bomandcargo_manifest::split_bom, and re-point their callers.formats::pnpm: havehead_lock_version,lock_versions,is_pnpm_lock_textandmay_need_store_flagreadstrip_bom(text). Have thepnpm-workspace.yamlsplices calltop_level_keyon a BOM-stripped first line, re-adding the BOM on write. Then makegoverning_root::workspace_lockfile_diratop_level_keycaller, which deletes its private key-prefix grammar.strip_prefix('\u{feff}')/trim_start_matches('\u{feff}')sites to the helper, file by file. Any site that keeps "any number of BOMs" must justify it in a comment.Size and scope
formats/,utils/serde.rs,gradle/dsl.rs,vendor/cargo_manifest.rsandhosted/governing_root.rs.pip freeze >writes) as absent: exit 0, no warning, and pip keeps installing the unpatched pin #721 / Fix UTF-16 requirements.txt silently skipped (#721) #724).Acceptance criteria
strip_bom/split_bompair informats::text, and the four named copies are deleted.sniff_lock_grammar,lock_version_major,is_pnpm_lockandentriesanswers as its plain twin (table-driven unit test).top_level_keyon a BOM first line returns the plain key, and a workspace splice keeps the BOM byte-exact (regression tests for Hosted pnpm scan skips thetrustLockfile: trueauto-config when pnpm-lock.yaml starts with a UTF-8 BOM, so pnpm 11/12 frozen installs fail with ERR_PNPM_TARBALL_URL_MISMATCH after a successful scan #903 and pnpm-workspace.yaml with a UTF-8 BOM: hosted and vendored miss the first top-level key and append a duplicatetrustLockfile/overrides, so every pnpm install fails with "duplicate mapping key" after a successful scan #904).parse_json_manifest_reads_past_one_bom_onlyand the existing CRLF/BOM round-trip tests (npm_lock_rewrite_keeps_crlf_tabs_and_bom,berry_crlf_and_bom_locks_round_trip_byte_exact,bom_prefixed_lock_is_rewritten_with_the_bom_intact) stay green.grep -rn "feff" crates/socket-patch-core/srcoutsideformats/text.rsand tests lists only sites with a justifying comment (after step 3).Dependencies
trustLockfile: trueauto-config when pnpm-lock.yaml starts with a UTF-8 BOM, so pnpm 11/12 frozen installs fail with ERR_PNPM_TARBALL_URL_MISMATCH after a successful scan #903, pnpm-workspace.yaml with a UTF-8 BOM: hosted and vendored miss the first top-level key and append a duplicatetrustLockfile/overrides, so every pnpm install fails with "duplicate mapping key" after a successful scan #904 and Hosted and vendored NuGet reject a packages.lock.json with a UTF-8 BOM that dotnet restores fine: hosted skips the redirect and exits 0 success, vendored fails apply_failed #623 one-line fixes.