Skip to content

Fix lockless nested npm projects in root scans (#1554) - #1591

Merged
Mikola Lysenko (mikolalysenko) merged 8 commits into
mainfrom
agent/fix-lockless-nested-npm-project
Oct 20, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 8 commits into
mainfrom
agent/fix-lockless-nested-npm-project

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1554

Summary

A root scan --mode vendored over a repo that holds an npm project installed without a lockfile (package-lock=false in its .npmrc, or npm install --no-package-lock) failed every run with vendor_lock_entry_not_found, exited 1 and dropped the in-run VEX. The suggested npm install couldn't clear it. Such a project is now treated like a nested project that has its own lock (#1534/#1543): its packages get a nested_project_skipped warning, and the root run succeeds and writes its VEX.

Root cause

nested_npm_project (crates/socket-patch-core/src/hosted/governing_root.rs) only counted a directory below the scan root as an independent project if it held its own npm-family lock. A lockless project was never recognised, so its copies stayed with the root's vendor backend, and the root lock never lists them.

Fix

  • nested_npm_project checks lock roots first, as before. If none matches, it falls back to lockless_npm_project. That function takes the directory that owns the copy's first node_modules and, as long as a workspace lists it, climbs to that workspace's root. The result counts as independent when it sits strictly below the scan root and has a package.json.
  • listing_workspace decides membership whether or not the workspace was ever locked. It reads package.json / vlt.json workspaces in every package manager's dialect, plus pnpm-workspace.yaml packages: via lists_as_member, which leans toward "member" when it can't tell. It also treats a Rush root as an owner. The walk stops at the scan root.
  • The nested_project_skipped warning for a lockless project now says the project has no lockfile. It offers remedies that work: run npm install --package-lock there and then scan it, or socket-patch scan --mode agent <dir>. The old text claimed the project had its own lockfile and pointed to a hosted/vendored rerun, which would be refused. docs/ecosystems.md is updated to match.

Tests (red → green)

Issue Test Without fix With fix
#1554 (core, incl. brace/unreadable pnpm globs after Bugbot) governing_root::tests::nested_npm_project_finds_a_lockless_project left: None, right: Some(".../tools/x") pass
#1554 (core: pnpm root, Rush, member scan) governing_root::tests::lockless_project_under_other_roots — (added after review) pass
#1554 (e2e: .npmrc package-lock=false and plain lockless; 2 runs + --vex) scan::scan_nested_project_e2e::root_vendored_scan_skips_a_lockless_nested_project npmrc=true run 1: exit 0 assertion fails (exit 1) pass

Commands run locally:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • cargo test -p socket-patch-core --lib governing_root: 45 passed
  • cargo test -p socket-patch-cli --all-features --test scan scan_nested_project: 7 passed
  • cargo test -p socket-patch-cli --all-features --test remedy_commands_parse: 2 passed (the new remedy commands parse)
  • cargo test --workspace --all-features --no-fail-fast: all pass except 10 tests that rely on a write or remove being denied (*_write_failure_*, *_unremovable_*, repair_cleanup_failure_*, relax_loop_must_not_traverse_symlinked_root, e2e_bun_lockb::binary_shared_bundled_record_hosted_pin_is_managed). The sandbox runs as uid 0, so those denials never happen there. None of them reaches nested_npm_project or the scan nested-project path. CI runs as non-root and is the check for them.
  • npm/pypi/gem wrappers: not touched, so no parallel change needed.

Bugbot

  • 7ce7fcd: one finding (an unmodeled pnpm glob counted as "member", hiding nested projects). Fixed in 80868e4 with a test; the thread is resolved.
  • 80868e4: one finding (the brace-glob fallback ignored pnpm's !, test and dot rules). Fixed in 121882e with a test; the thread is resolved. CI on 80868e4: 34 pass, 0 fail.
  • 121882e: one low-severity finding (every leading ! was stripped). Fixed in 6f4c327; the thread is resolved.
  • 6f4c327: Bugbot found no new issues. CI: 35 pass, 19 skipped, 0 fail.
  • CI on 7ce7fcd: 35 pass, 50 skipped, 0 fail. That includes the root-only tests listed above.

/code-review high

I fixed every confirmed finding except two:

🤖 Generated with Claude Code

https://claude.ai/code/session_01LYN2ka9heR9fMVjsgTJGfZ


Generated by Claude Code


Note

Medium Risk
Changes nested-project detection and skip/warning paths for hosted/vendored npm scans; incorrect workspace or lockless classification could skip or patch the wrong packages, though coverage is broad.

Overview
Root vendored/hosted scans no longer fail with vendor_lock_entry_not_found when a nested directory has package.json and node_modules but no lockfile (package-lock=false or npm install --no-package-lock). Those installs are skipped like nested projects with their own lock (#1534).

nested_npm_project in governing_root.rs still prefers directories with an npm-family lock (or Rush). When none match, it uses new lockless_npm_project logic: walk from the copy’s first node_modules up to the scan root, climbing to a workspace root when listing_workspace lists the path (npm/yarn/vlt/pnpm/Rush, including partial pnpm glob handling). has_own_npm_family_lock is exported for discovery warnings.

nested_project_skipped text now splits locked vs lockless nested projects: lockless warnings suggest npm install --package-lock then a targeted scan, or socket-patch scan --mode agent <dir>. Docs in ecosystems.md match.

CI pins uv 0.13.1 in ci.yml and pipenv-compatibility.yml because 0.13.2 rejects Pipenv pylocks with environments = [].

Unit tests in governing_root and e2e root_vendored_scan_skips_a_lockless_nested_project cover the behavior.

Reviewed by Cursor Bugbot for commit e7987ac. Configure here.

Assisted-by: Claude Code:claude-opus-5-5
A root `scan --mode vendored` over a repo that holds an npm project
installed without a lockfile (`package-lock=false` in its .npmrc, or
`npm install --no-package-lock`) handed that project's packages to the
root's lockfile. The root lock never lists them, so every run failed
with vendor_lock_entry_not_found, exited 1 and dropped the in-run VEX,
and the suggested `npm install` could not clear it.

Such a directory, with its own package.json and node_modules and not
listed by any workspace, is now treated like a nested project with its
own lock: its packages get the nested_project_skipped warning naming
the PATH to scan, and the root run succeeds. A member of a lockless
inner workspace counts as part of that workspace's project.

Fixes #1554

Assisted-by: Claude Code:claude-opus-5-5
The nested_project_skipped warning for a project installed without a
lockfile said it had its own lockfile and told the user to run a
hosted or vendored scan of it, which is refused there for lack of a
lock. It now says the project has no lockfile and offers two remedies
that work: create the lock with `npm install --package-lock` and scan
it, or patch its node_modules in place with agent mode.

The lockless check also stops at the scan root, so a workspace file
above the directory being scanned no longer decides what counts as
nested below it; a pnpm workspace root no longer claims a directory it
does not list; and Rush projects stay the Rush root's.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 15, 2026 21:43
@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.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/hosted/governing_root.rs Outdated
A pnpm-workspace.yaml whose packages: globs use braces or classes
(`{apps,libs}/*`), or that could not be read, made every lockless
project below it count as a workspace member, so a root vendored scan
still failed on it. Such globs are now matched with the shared
workspace glob grammar, and an unreadable file lists nothing, since
pnpm cannot install from it either.

Assisted-by: Claude Code:claude-opus-5-5
@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.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/hosted/governing_root.rs Outdated
The brace-glob fallback for pnpm-workspace.yaml matched a directory if
any package manager's reading did, so a `!` exclusion, a `test` or
`tests` directory, or a dot directory could still count as a member,
and a lockless project there failed the root vendored scan. The
fallback now reads each glob pnpm's way.

Assisted-by: Claude Code:claude-opus-5-5
@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.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/hosted/governing_root.rs Outdated
The brace-glob fallback stripped every leading `!`, so `!!apps/x`
excluded apps/x, while pnpm's own matcher strips one `!` and treats
the rest as a literal that matches nothing. It now strips one.

Assisted-by: Claude Code:claude-opus-5-5
@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.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 16, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review — head 6f4c327.

Slack announcement not sent this run (send unavailable); the next run retries.


Generated by Claude Code

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 20, 2026
The nightly e2e_vex_build pipenv legs (ubuntu and macOS) went red on
2026-10-20: `uv pip install -r pylock.toml` refuses the pylock Pipenv
2026.8.0 writes with "The target environment is not supported by the
lockfile". Pipenv emits `environments = []`, and uv 0.13.2 (released
2026-10-19) started treating an empty list as "no environment
supported". setup-uv installs the latest uv when no version is given,
so the oracle drifted under an unchanged main.

Pin uv to 0.13.1, the last version these legs passed on, in ci.yml
and pipenv-compatibility.yml, as the other workflows already pin
their uv.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H4ww7ytRoApPcBHiJN4bm2
(cherry picked from commit a7c9207)
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] The merge queue removed this PR because of e2e (ubuntu-latest, e2e_vex_build, pipenv:: --ignored, 2026.8.0) (pipenv::pipenv_pylock_projects_wire_what_pipenv_installs: uv pip install -r pylock.toml → "The target environment is not supported by the lockfile").

This PR did not cause it. The pipenv legs install whatever uv is latest, which was 0.13.1 in yesterday's passing queue run and 0.13.4 in today's. uv ≥ 0.13.2 refuses the environments = [] that Pipenv 2026.8.0 writes, so the leg fails for every PR in the queue, and main's nightly fails the same way. #1639 has the diagnosis and the fix (pin setup-uv to 0.13.1).

e7987ac merges main and ports #1639's commit (a7c9207, cherry-picked), so this PR can go back into the queue now. Once #1639 merges, the port changes nothing. Checked locally after the merge: fmt, clippy -D warnings, governing_root unit tests and scan_nested_project e2e tests all pass.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Wenxin Jiang (@Wenxin-Jiang) this changed after your approval (on 6f4c3273), so it needs another look before it goes through the merge queue.

Besides merges from main, the only new commit is e7987ac: the CI-only uv pin from #1639, byte-for-byte the same 12 added lines (version: "0.13.1" on the two setup-uv steps in ci.yml and pipenv-compatibility.yml). No crate code changed. It's there because uv 0.13.2 fails the pipenv:: e2e leg on every merge-queue entry. Once #1639 lands it does nothing.

If that's fine with you, re-approving e7987ac4 is all this needs.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 20, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] The merge queue removed this PR again, this time for coverage: one test, update::swap::tests::update_lock_is_exclusive_and_released_on_drop, panicked at crates/socket-patch-core/src/update/swap.rs:467 (unwrap() on Err(InProgress), the re-acquire after drop(first)). The other 6547 core tests passed.

This PR did not cause it:

Every test that sets SOCKET_UPDATE_STATE_DIR shares #[serial(update_state_dir_env)]. The likely race: the flock is per open file description, so a child process another test spawns in parallel holds a copy of the lock fd from fork until exec (O_CLOEXEC only closes it at exec). If that child is in that window when first drops, the re-acquire still sees the lock held.

No fix exists yet. A proposed patch for a separate PR is to let the re-acquire in that test retry briefly:

let reacquired = (0..50).find_map(|_| match acquire_update_lock() {
    Err(UpdateError::InProgress) => { std::thread::sleep(std::time::Duration::from_millis(20)); None }
    other => Some(other),
});
assert!(matches!(reacquired, Some(Ok(Some(_)))), "lock must be reacquirable after release");

I am putting this PR back in the queue once. If it fails a second time, I will treat that as a real failure.


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 e7987ac. Configure here.

Merged via the queue into main with commit 1a1294b Oct 20, 2026
55 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-lockless-nested-npm-project branch October 20, 2026 18:46
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