Repository navigation
Fix yarn classic copy identity read two ways (#1951) - #1984
Conversation
Assisted-by: Claude Code:claude-opus-5-5
Hosted scans name a yarn classic file: directory copy through their own reader, which drops a copy outside the project root (file:../x) that vendored and VEX read and name. Add regression tests for the rewriter and the hosted manifest read, plus the shared reader they will use. Refs #1951 Assisted-by: Claude Code:claude-opus-5-5
A hosted scan named a yarn classic file: directory or url copy through its own reader in formats/yarn/source.rs, which dropped a directory outside the project root (file:../ext/x). Vendored and VEX read that copy and named it, so hosted reported a clean pin while yarn kept installing the unpatched out-of-root copy, without the directory-skipped warning vendored gives. Hosted now reads the copy's manifest and identity through formats::yarn::copy (classic_copy_ref, CopyRef::read_path and identity), the same reader vendored and VEX use: out-of-root copies are read, absolute paths never are. The second reader and its third dir/package.json join are deleted. Fixes #1951 Assisted-by: Claude Code:claude-opus-5-5
A yarn classic file: directory copy outside the project root is now named by the yarn rewriter, with the same redirect_yarn_classic_directory_skipped warning an in-root copy gets, instead of the engine's generic redirect_copy_unpatched fallback. The e2e now asserts that code, and that the copy is named exactly once. The rewriter test's doc says what it compares: which package the copy holds. Refs #1951 Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ 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 0c263ae. Configure here.
|
[burn-down agent] Labeled Ready for review at 0c263ae.
Generated by Claude Code |
Final review briefWhat it does. Hosted scans now read which package a yarn classic Risk: low. Narrow change in one rewriter's identity lookup. The one widening is that hosted now reads Look here
Verified
Changes I made: none. Open questions: none blocking. The PR body lists pre-existing gaps it leaves alone (hosted still skips an unidentified copy that vendored keeps as a possible copy; Auto-merge is armed, so approving sends it straight to the merge queue. Generated by Claude Code |
Wenxin Jiang (Wenxin-Jiang)
left a comment
There was a problem hiding this comment.
[agent] Reviewed 0c263aedcbccdd28b505b97cf6378a072f5943c6. Checked shared Yarn classic copy identity and advisory directory-manifest reads. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1951
Summary
Hosted scans now identify a yarn classic
file:directory or tarball-URL copy throughformats::yarn::copy(classic_copy_ref,CopyRef::read_path,CopyRef::identity). That is the reader vendored and VEX already use. The second reader informats/yarn/source.rs(classic_file_directory,classic_copy_real_name) and its thirddir/package.jsonjoin are deleted.Root cause
classic_file_directorynormalized thefile:path through a Cargo helper (cargo_workspace::normalize_rel) that drops any path leaving the root. Forlp2@file:../ext/lpdir, hosted never read the copy'spackage.json, so the yarn rewriter never named it. Only the engine's generic post-run fallback (redirect_copy_unpatched) caught it, whereas an in-root copy getsredirect_yarn_classic_directory_skippedand is counted inbundled_skipped_uuids. Vendored and VEX read the same copy (#1516), so the modes disagreed.Changes
formats/yarn/copy.rs: newclassic_copy_name(patterns, resolved, version, read_text)overclassic_copy_ref+identity. It is gated to theDirectoryandRemoteTarball-URL copies the old function handled; afile:tarball still needs bytes, which a text reader can't give.hosted/engine.rs: reads each directory copy's manifest atCopyRef::read_path(). Out-of-root paths are read, as vendored and VEX read them; absolute paths are never read.patch/redirect/mod.rs: the yarn classic rewriter callsclassic_copy_name.formats/yarn/source.rs: the duplicate reader is deleted.e2e_redirect_yarn_classic_build:classic_outside_root_copy_under_another_name_is_named_and_not_attestednow expects the out-of-root copy to be named exactly once, with the code an in-root copy gets (redirect_yarn_classic_directory_skipped). The VEX assertions are unchanged.Test evidence
patch::redirect::tests::issue_1951_yarn_classic_copy_named_as_vendored_reads_it(in-root,file:../ext,file:/absand codeload: hosted names a copy iff vendored identifies it)out-of-root directory: [])hosted::engine::tests::issue_1951_yarn_classic_copy_manifests_read_as_vendored_reads_them(disk view reads../ext/lpdir/package.json, never the absolute one)None)Run locally on
0c263ae:cargo fmt --all -- --check: ok.cargo clippy --workspace --all-features -- -D warnings: ok.cargo test -p socket-patch-core --all-features: 6997 passed. 4 failed, and the same 4 fail onmainin this container, because it runs as uid 0, so chmod-based write-failure tests can't fail:copy_tree::relax_loop_must_not_traverse_symlinked_root,vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry,pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouchedandpypi_requirements::wire_failure_rolls_back_already_written_files.cargo test -p socket-patch-cli --all-features --lib: 940 passed.scripts/yarn-classic-vex-matrix.sh 1.22.22: every VEX cell passed.e2e_redirect_yarn_classic_buildfailed only on the out-of-root test above, as expected, and passes 42/42 after the update.cargo test --workspaceran out of the container's disk building all CLI test binaries, so CI covers the rest of the CLI integration suites.No wrapper changes are needed:
npm/,pypi/andgem/only dispatch to the binary./code-review high: findings not fixed here
file:, unreadable manifest, codeload): vendored keeps it as a possible copy (Yarn classic renamed-copy detection (#1489) misses afile:copy outside the project root and a tarball whose root isn'tpackage/, so scans succeed and VEX attests while that copy installs unpatched #1516). This predates the PR; the old reader returnedNonefor the same inputs. It is a separate rule (possible-copy naming), not the identity reader this issue consolidates. Worth a follow-up issue.file:tarball copies keyed under another name aren't identified by the hosted rewriter, because the engine's file map is text-only. This predates the PR and is unchanged.live == false) blocks: the hosted rewriter doesn't filter on liveness. This predates the PR and is unchanged.file:copy outside the project root and a tarball whose root isn'tpackage/, so scans succeed and VEX attests while that copy installs unpatched #1516); only the manifest'snameis used, and absolute paths are still never read.classic_copy_namegate, and perf of per-block reads: both gates take the firstfile:pattern viaclassic_copy_refand agree on the mixed tarball+directory case (both skip it). The per-block reads matched what the old code did for in-root copies.https://claude.ai/code/session_011f9aeRqL3jx1o1HYCU4NK4
Generated by Claude Code