Fix agent mode patching linked first-party source (#626)

[Mikola Lysenko (mikolalysenko)]

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

Fixes #626

Summary

Agent-mode apply and rollback no longer write through a node_modules/<name> (or node_modules/@scope/<name>) link whose real path is outside every node_modules tree. Package managers link that way only to first-party source: an npm, Yarn, pnpm or Bun workspace member, a file: or link: directory dependency, or an npm link target. Before this change, when such a local package shared a patched package's name@version, the default mismatch policy replaced the user's own code with upstream bytes. vex then attested the patch, and rollback wrote the upstream original over the fork. Now both commands fail closed, dry run included. The error names the real path and says to patch that source directly, which matches vendored mode's vendor_workspace_member refusal.

Root cause

The npm crawler accepts any symlink (or Windows junction) in an importer tree as a package copy. That is meant for pnpm/vlt/Yarn store links, but it never checks where the link resolves. The patch engine then commits through whatever path it was given.

Fix

Agent apply and rollback already have one boundary for package dirs they must not write in place: patch::shared_store. It refuses pnpm's global virtual store and PDM's symlink cache, and it checks the package dir and every patched file's parent, dry run included. This PR adds a third kind there, SharedStoreKind::LinkedSource, which matches when:

• the path is spelled as a node_modules entry (node_modules/<name> or node_modules/@scope/<name>), and
• its real path is neither inside that node_modules (a real dir, or a link into its own .pnpm / .store / .vlt / .bun store, even when node_modules itself is a symlink) nor below any other node_modules (a workspace member's link into the root .pnpm).

Store links and real dirs are patched exactly as before. That includes Yarn's pnpm-linker store relocated outside node_modules via pnpmStoreFolder, but only for an active Yarn pnpm install (a yarn.lock plus nodeLinker: pnpm), only to the <store>/<entry>/package dir of a package Yarn installs as a copy (npm, virtual, file, patch, http(s), git; never workspace/portal/link), and only when the store does not contain the project. Codex review rounds drove this: 929db7e added the exception, 3ac1213 closed a stray-config hole that would have let an npm workspace member through, f419eb5 accepts peer-instantiated (virtual:) entries such as react-dom, and cf1b6e9 accepts the other copy protocols, such as a scoped file: tarball. Docs: docs/ecosystems.md describes the new refusal next to the shared-store one. Hosted and vendored modes, and the npm/PyPI/gem wrappers, are unaffected (they don't use this path).

I chose #626 over the two-issue #628/#629 cluster in the same tier: #626 is silent loss of committed first-party code that no reinstall restores, while #628/#629 is diff noise plus a refactor.

Test evidence

Each test below was run red with the detection disabled (linked_source_of returning None) and green with the fix:

• patch::shared_store::tests::node_modules_link_to_first_party_source_is_refused: workspace member, scoped member, an npm link target outside the project, and a global-prefix npm link. Red → green.
• patch::apply::tests::test_apply_refuses_node_modules_link_to_first_party_source: member, scoped member and out-of-project link, for policies Warn and Force, dry run and real. The fork keeps its bytes. Red → green.
• patch::rollback::tests::test_rollback_refuses_node_modules_link_to_first_party_source: a fork left patched by a pre-fix apply is not overwritten with the upstream original. Red → green.
• apply suite in_process_npm_multicopy::apply_and_rollback_refuse_a_node_modules_link_to_first_party_source: the real binary on an npm-workspace tree; apply --dry-run, apply and rollback exit non-zero, the JSON envelope names the cause, and the member's source is untouched. Red → green.
• relocated_yarn_pnpm_store_is_not_refused: a native relocated store stays patchable. Refused without a yarn.lock, with nodeLinker: node-modules, with pnpmStoreFolder: ., for a non-package store link and for a workspace-slug entry. react-dom-virtual-… and scoped @acme-tool-file-… entries stay patchable. Red → green.
• stray_yarn_store_setting_does_not_admit_an_npm_workspace_member: the review's reproduction. An npm workspace with a stray .yarnrc.yml (pnpmStoreFolder: packages) links node_modules/left-pad to packages/foo/package, and the link stays refused. Red → green.
• Negative test node_modules_store_links_and_real_dirs_are_not_refused: real dirs, scoped real dirs, a member link into the root .pnpm, Yarn's .store/<entry>/package, and a symlinked node_modules root with both real and .store entries. All stay patchable. Existing test_apply_patches_through_per_project_pnpm_link, the vlt/npm multi-copy suites and the shared-store tests still pass.

Local runs:

• rustfmt: the touched files are rustfmt-clean. CI has no fmt job, and main itself is not fmt-clean under the pinned toolchain, so the reformatting an earlier commit swept in was reverted in 2feeaa5 to keep this PR scoped.
• cargo clippy --workspace --all-features -- -D warnings: clean.
• cargo test --workspace --all-features --no-fail-fast: all pass except tests that can't run in this container. These fail identically on main's versions of the touched files. They are the chmod 0o555 write-failure tests (the container runs as root, which ignores the mode): covgap_commands_vendor::*state_write_failure*, in_process_redirect write-failure tests, repair cleanup-failure tests, copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock… and pypi_*::wire_*failure*. The remaining failures were update-fixture, vlt_lock and registry_fetch tests that hit a full disk mid-run; the core ones pass on re-run. CI (non-root runners) is the authority for those.
• No ecosystem e2e suite was run locally. The change is in the shared apply/rollback engine, and CI's npm-family legs exercise it.

CI: all checks green on cf1b6e9 (402 passed, 6 skipped by path filters). Bugbot found no issues on any head. Codex re-review of cf1b6e9 accepted all four of its findings as fixed: it ran 126 focused tests and 28 native CLI operations against real Yarn 4.12, npm 11.19 and Node 24 installs.

Per-issue checklist

• Agent-mode apply writes through node_modules links into first-party source (npm workspace members, file: deps, npm link targets), overwriting the user's code, and rollback restores upstream bytes instead #626, workspace member: apply/rollback/CLI tests above
• Agent-mode apply writes through node_modules links into first-party source (npm workspace members, file: deps, npm link targets), overwriting the user's code, and rollback restores upstream bytes instead #626, file: directory dependency (same on-disk shape as a member link): shared_store + apply tests
• Agent-mode apply writes through node_modules links into first-party source (npm workspace members, file: deps, npm link targets), overwriting the user's code, and rollback restores upstream bytes instead #626, npm link target outside the project, including via the global prefix: shared_store + apply tests
• Agent-mode apply writes through node_modules links into first-party source (npm workspace members, file: deps, npm link targets), overwriting the user's code, and rollback restores upstream bytes instead #626, Windows junction: same code path (canonicalize resolves junctions); the tests are cfg(unix) like the existing shared-store tests, and Windows CI runs the rest

Follow-ups

• Agent-mode apply in a pnpm workspace reports each member-linked package twice, inflating the --json skipped count with duplicate already_patched events #633 (pnpm member-linked copies reported twice) is a different code path (ecosystem_dispatch.rs push_path literal-path dedup) and is left for its own fix.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SoWUfwR22TaCBa9tCuJGpU

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Burn-down agent: Ready for review at 2feeaa5.

• CI: 97/97 non-skipped checks green on the head SHA (3 skipped by path filters); mergeable with main.
• Bugbot: reviewed 2feeaa5, no issues found; no unresolved review threads.
• Reviewer focus: the behaviour change described in the summary; no outstanding findings.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Codex review of cf1b6e9d42a1e869ff58657189e6a56bebeaaf94: all reported findings are fixed; ready to merge as-is from this review.

The author’s latest commits address all four reproduced cases: relocated Yarn pnpm registry copies remain patchable, unused Yarn configuration no longer admits the npm workspace overwrite, peer-instantiated registry packages are accepted, and copied tarball dependencies are accepted. The exception remains limited to the documented Yarn configuration and installed-entry forms; source-link protocols stay refused.

Verified on a clean, unchanged checkout of this exact commit: 126 focused tests and 28 native CLI operations passed, using native Yarn 4.12.0, npm 11.19.0 and Node 24.21.0 installations. Apply/rollback, including dry runs, work for ordinary, peer and scoped tarball copies. npm and Yarn first-party workspace refusals preserve the source bytes; copied-tarball operations preserve the source archive. The fresh binary and all six PR-changed file hashes were checked against this commit. Changed-file formatting and targeted Clippy pass (with the existing macOS unused_variables allowance), and the branch merges cleanly with main 045d7ec.

Independent final source review found no remaining actionable issue. The author correction is accepted; no additional code change is required. 402 successful checks, 7 skipped, and 9 successful workflows (one additional workflow skipped). Bugbot is clear on this commit; no unresolved review threads or new actionable findings. Ready for review has been restored. GitHub still requires the normal human approval before merge.

[Mikola Lysenko (mikolalysenko)]

Re the Codex P2 (relocated Yarn pnpm store): confirmed and fixed in 929db7e.

• linked_source_of now also accepts a link whose real path is exactly <pnpmStoreFolder>/<entry>/package. The store comes from the nearest .yarnrc.yml at or above the project, resolved against that file's directory as Yarn does.
• It doesn't loosen the guard for first-party source. A store folder that contains the project (pnpmStoreFolder: .) is ignored. A link into the store that isn't an entry's package dir is still refused, and so are workspace members linked beside a relocated store.
• The regression test relocated_yarn_pnpm_store_is_not_refused covers an ancestor rc versus the project's own rc, a quoted value and a trailing comment, plus the cases above. It fails with the new check disabled and passes with it.
• docs/ecosystems.md and the module doc now describe the exception.

No need for a separate correction from the review agent; this push covers it.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Re the Codex follow-up P1 (an inactive Yarn config could admit an npm workspace member): confirmed, and fixed in 3ac1213, built on 929db7e and keeping the yarnrc_scalar refactor.

in_yarn_pnpm_store now accepts a link only when all of these hold:

• Yarn is in use: a yarn.lock exists at or above the project.
• The pnpm linker is active: the nearest .yarnrc.yml that sets nodeLinker sets it to pnpm.
• The store is separate: pnpmStoreFolder comes from the nearest .yarnrc.yml that sets it, and that folder does not contain the project.
• The target is an installed registry package: it is exactly <store>/<ident>-npm-<version>-<10 hex>/package, the layout Yarn gives a hard registry package. Workspace and other soft locators never get it.

Tests:

• stray_yarn_store_setting_does_not_admit_an_npm_workspace_member is your reproduction: an npm workspace with package-lock.json, a .yarnrc.yml containing nodeLinker: node-modules and pnpmStoreFolder: packages, and node_modules/left-pad linked to packages/foo/package. That link is refused, and so is a member dir named like a registry slug. The link stays refused when the config says nodeLinker: pnpm but there's no yarn.lock.
• relocated_yarn_pnpm_store_is_not_refused checks that a native relocated store still patches. The link is refused without a yarn.lock, with nodeLinker: node-modules, with pnpmStoreFolder: ., for a non-package store link and for a workspace-slug entry.
• yarn_registry_slugs covers the slug matcher.
• Both store tests fail with the three new requirements removed and pass with them.

Local runs: clippy is clean, the CLI apply suite passes, and the patch/vendor library tests pass except the 4 permission tests that can't pass in this container (it runs as root), as listed in the PR body.

Since this push covers the correction, a second one from the review agent shouldn't be needed. A re-review of 3ac1213 is welcome.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Re the Codex P2 on 3ac1213 (peer-instantiated packages such as react-dom-virtual-685e277730 were refused): confirmed, and fixed in f419eb5.

• is_yarn_registry_slug now accepts <ident>-virtual-<10 hex> as well as <ident>-npm-<version>-<10 hex>. That is the slug Yarn's slugifyLocator gives a virtual: locator.
• This admits no first-party source. Under the pnpm linker, workspaces (virtual ones included) are SOFT links straight to their source and never get a store entry. Every earlier condition still applies: a yarn.lock, an active nodeLinker: pnpm, a store that doesn't contain the project, and an exact <store>/<entry>/package target.
• relocated_yarn_pnpm_store_is_not_refused now also checks that a react-dom-virtual-…/package entry stays patchable. yarn_registry_slugs covers the virtual forms, including scoped ones, and rejects malformed ones (no ident, bad hash).
• Both tests fail without the virtual branch and pass with it. Clippy is clean and the CLI apply suite passes.

This push covers the correction, so the separately prepared one shouldn't be needed. A re-review of f419eb5 is welcome.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Codex follow-up coordination on f419eb5: the added virtual form addresses the peer-package finding. One already-reproduced case still remains: a scoped file: tarball dependency is copied by native Yarn 4.12 into @review-scoped-file-1e780c037d/package beside a real node_modules. It is an installed HARD copy, and patching it leaves the source tarball untouched. The current npm/virtual slug predicate excludes that entry. The same copied dependency remains patchable in the default store.

The real CLI refused all four apply/rollback wet/dry operations on this fixture at 3ac1213; the prepared layout-based correction passed all four with the tarball unchanged. The f419eb5 delta only adds virtual slugs, so it leaves this file case excluded. I am handling this remaining follow-up on the latest author head, retaining the author’s source-refusal regressions and adding durable CLI coverage for ordinary, scoped, workspace-importer, peer, and copied-tarball installations. The inactive-config npm workspace refusal stays covered too.

The correction identifies a configured project-owned installed layout and documents conservative refusal of ambiguous configuration. I will push it after final reconciliation and native verification, then wait for full CI before restoring Ready for review.

[Mikola Lysenko (mikolalysenko)]

Follow-up on the edited Codex review of 3ac1213: f419eb5 covered the virtual: peer case but not the scoped file: tarball copy. That case is fixed in cf1b6e9.

• Yarn slugs a store entry as <ident>-<protocol>-<10 hex>, with npm-<version> in place of the protocol for npm: locators. The matcher, now named is_yarn_copy_slug, accepts the protocols Yarn installs as a copy: npm, virtual, file, patch, http, https, git and github.
• It still refuses workspace, portal, link and any unknown protocol. The first three link to source and never get a store entry. All the earlier conditions still apply: a yarn.lock, an active nodeLinker: pnpm, a store that doesn't contain the project, and an exact <store>/<entry>/package target.
• Tests: relocated_yarn_pnpm_store_is_not_refused now also checks that a scoped @acme-tool-file-…/package entry stays patchable. yarn_copy_slugs covers the file, patch and https forms and the refused workspace, portal, link and exotic forms. Both fail with only virtual allowed and pass with the full list. Clippy is clean and the CLI apply suite passes.

If the separately prepared correction covers a layout these tests don't, please name it and I'll add it.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 cf1b6e9. Configure here.}