Conversation
amit-turgeman
requested review from
avishaiamiel and
omer-roth
as code owners
September 28, 2026 16:35
Collaborator
|
omer-roth
reviewed
Sep 30, 2026
omer-roth
reviewed
Sep 30, 2026
omer-roth
requested changes
Sep 30, 2026
omer-roth
previously approved these changes
Oct 1, 2026
npm installs a workspace from the root lockfile and ignores any lockfile inside a member, so the one we generate here is uploaded and never used. Worse, generating it re-resolves the member's transitive ranges against the registry, so the scan sees versions the repository does not install. A directory only counts as the workspace root when its package.json declares a "workspaces" pattern matching the member and its lockfile lists that member. A lockfile entry on its own is not enough: npm records a file: dependency exactly like a workspace member, and a file: target is not resolved through the root lockfile, so it still needs a lockfile of its own. Covers both lockfile shapes that can describe a workspace, the array and object forms of "workspaces", and npm-shrinkwrap.json. A lockfileVersion 1 root has no "packages" map and predates workspaces, so it keeps generating as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The npm fallback generated a package-lock.json for a member of a yarn, pnpm or bun workspace. The dedicated handlers only look for their lockfile in the member's own folder, and a workspace keeps it solely at the root, so nobody claimed the member and npm took it — wrong package manager, and versions re-resolved against the registry rather than the ones installed. Coverage detection moves into a shared module that all four handlers consult. npm still requires the member to appear in the root lockfile, so a stale root cannot hide it; yarn, pnpm and bun cannot be enumerated cheaply or uniformly, so for those the root lockfile's presence is the signal. pnpm declares its members in pnpm-workspace.yaml, and each package manager resolves membership only against the declaration file it actually reads — a stale workspaces array left behind by a migration does not make a pnpm member. Negated globs are honoured, so an excluded directory still gets its own lockfile instead of being silently dropped. The root lockfile is parsed once per scan rather than once per member. Only the derived member-name set is retained, not the parsed document: for a 24 MB lockfile that is 8 KB instead of 98 MB. Deciding what to skip across 200 members drops from 19.4s to 0.1s. A committed npm-shrinkwrap.json is now used instead of being regenerated, and the collected document keeps that name rather than being reported as a package-lock.json. The ancestor walk stops at the git repository root, falling back to the scanned path when there is none, so a manifest outside the scanned tree cannot suppress a project inside it. Scanning only a member folder still declines, and says so once with the root it found. bun.lockb counts as workspace coverage but is deliberately not treated as an alternative lockfile in the member's own folder: Bun restores only from a text bun.lock, so excluding it there would leave a Bun <1.2 project with no handler at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
workspace.py declared its own copies of the file names the handlers already had, so the coverage table and the handler consuming it could drift apart without anything failing. package.json existed five times, and yarn.lock, pnpm-lock.yaml, bun.lock and deno.lock three times each. workspace.py now owns them and the handlers import from it, keeping their existing constant names so callers are unaffected. A test scans the module for a bare file-name literal and names the offending file, so the convention is enforced rather than remembered. The npm alternative-lockfile list is built from those constants, which makes the absence of the binary bun lockfile visible in the code instead of needing a comment to explain it, and a test pins that exception. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects found in review. scan_roots_from_context kept only str entries, but typer declares these arguments as Path with resolve_path=True, so every real scan root was discarded and the tuple was always empty. The containment check short circuits on an empty tuple, so the "workspace root is outside the scanned path" warning could never fire in production and the boundary fallback for a tree without .git never applied. Scan roots are now taken from any PathLike, and both the single path and the list of paths are read. The previous tests passed a str, which is the one shape typer never produces. Containment compared os.path.abspath, which does not resolve symlinks, while typer hands back a resolved path. A scan root reached through a symlink therefore looked as if it sat outside itself, which would have produced a spurious warning as soon as the above was fixed. On macOS /tmp and /var are symlinks, so this is the ordinary case. Both sides are now compared as real paths. pnpm was treated as presence-only on the grounds that its lockfile cannot be enumerated, which is true of yarn and bun but not of pnpm: pnpm-lock.yaml carries a top-level importers map keyed by member directory, as checkable as npm's packages. A member declared in pnpm-workspace.yaml but missing from a stale lockfile was silently dropped. pnpm now requires lockfile membership. Only the importers block is parsed, which on a 2.4 MB lockfile costs 164 ms instead of 1369 ms for the whole document. The reasoning in the previous commit message holds for yarn and bun only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-root lookup The pnpm importers block is sliced by line, and any line starting at column zero ended it — including a comment. A hand-written comment inside importers therefore truncated the slice to nothing, and with no members found npm took the project and regenerated a package-lock.json for a pnpm workspace. Lines beginning with a hash are now skipped. pnpm does not emit such comments itself, so this was reachable only through hand editing. The scan-root lookup existed twice. get_path_from_context read the same two parameters as the newer function but kept whatever typer produced, so it returned a Path despite being annotated Optional[str], and it raised IndexError when paths was an empty list. The lookup now lives in path_utils, get_path_from_context returns its first entry, and the npm handlers call it there. Tests cover both the shapes typer produces and the empty list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Treating a root lockfile as coverage when that lockfile is outside the scanned paths produced no dependency documents at all. Running cycode scan path ./packages/app inside a monorepo found the root through the .git boundary, skipped the member, and then never collected the root lockfile, so the scan reported clean. Before this branch the same command produced detections. Wrong versions are bad; no data is worse. Such a root is no longer treated as coverage, the member is restored on its own, and the warning says which root to scan for the versions actually installed. The walk could also leave the scanned tree entirely. A scanned path may be a file rather than a directory, and comparing a file against a directory never matched, so no scan root appeared to contain the member and the walk ran to the filesystem root, reading manifests and lockfiles above it. An unrelated ancestor declaring workspaces ["**"] was accepted as the covering root and suppressed the restore. A scanned path now stands for its directory, and when the scanned paths are known the walk never passes them. Reading the pnpm importers block no longer loads the whole lockfile. The slice existed to avoid parsing a large document, but the file was still read and split in full: 44.6 ms and 37.5 MB for a 13.2 MB lockfile, against 0.3 ms and 0.1 MB when the read stops at the packages block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…st globs A lockfile names the directories it resolved, so it can answer directly whether a member is already covered: package-lock.json and npm-shrinkwrap.json in the keys of "packages", pnpm-lock.yaml in "importers", and a berry yarn.lock in the resolution of each "<name>@workspace:<path>" entry. Asking it removes a whole layer of inference. dependency-collector resolves the same question the same way, so the two now agree by construction rather than by coincidence. The globs were introduced to separate a workspace member from a file: dependency target, on the grounds that the root lockfile does not resolve the latter. Checked against npm 11, that is not so: a file: target gets a packages entry and has its ranges pinned under the root node_modules exactly as a member does. A second lockfile inside such a target can only drift from the root, so these are now skipped like any other directory the root resolves. The manifest globs remain for the two formats that cannot name their members: a classic yarn.lock, which is flat, and the binary bun.lockb. A v1 package-lock.json is a third case and must not reach them - workspaces arrived with npm 7 and lockfileVersion 2, so a v1 root is simply not a workspace. Each format is now a resolver class behind one abstract base, which caches and leaves the subclass to parse. Reading members and deciding what that format's silence means live together per format, so the yarn classic and berry split is stated in the class that handles it. This also closes two gaps the globs left. A pnpm root declaring src/app/** did not match src/app itself, though pnpm's own importers list it, and a berry member missing from the lockfile was skipped on the strength of the glob alone, collecting nothing for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The module had grown to five concerns in one file: per-format lockfile reading, glob matching, path and walk-boundary handling, caching, and the public entry points. Each is now its own module, with the dependencies running one way. names.py file names and package-manager identifiers files.py file identity for caching, and tolerant reading globs.py the manifest globs, reached only by the blind formats resolvers.py the abstract reader and one implementation per format coverage.py scan roots, the walk, and the two public functions __init__.py the public surface, re-exported Nothing outside the package changed: __init__ re-exports every name the handlers already imported, so no import statement elsewhere moved. The three caches now live beside the code that fills them, and clear_cache fans out to them, which keeps a caller from needing to know how many there are. Tests reach for the module that owns a helper rather than for one module that owned everything. Note that resolvers binds read_json_object at import, so a test patches the name there rather than on files. No behaviour change: the decision is identical across all 41 manifests of sca-npm-examples, and against the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The glob matcher built a regular expression a character at a time, with the trailing /** handled as a special case so that src/app/** would match src/app. Matching segment by segment says the same thing in a quarter of the code: a separator cannot occur inside a segment, so fnmatch is exactly right there and brings character classes with it, while ** is the only construct that spans segments. The directory matching its own /** pattern then follows from ** spanning zero segments, rather than from a special case. Checked against yarn 1.22, which is what the globs exist to mirror: *, **, ?, literals and character classes all agree. Brace expansion and extglobs are still unsupported and fail as a miss, which restores the member rather than dropping it. Each ** tries every split of the remaining path, so several in one pattern multiplied out: six globstars against a deep tree took 67ms, and twenty would not have finished. The pattern comes from a manifest inside the repository being scanned, so that was reachable from a crafted package.json. The match is now memoised on its two tuples of segments, which are plain values and cannot go stale, and twenty globstars take 0.35ms. bun.lock names its members under "workspaces", so bun no longer depends on the globs at all. It is JSON with trailing commas, which strict parsing rejects, so a failed parse retries once with those removed. The binary bun.lockb cannot be read and still falls back to the globs. deno.lock is no longer treated as covering a member. The fallback reads the manifest's workspaces field, which deno does not use - it declares members in deno.json - so a match there was meaningless. A deno.lock beside a manifest still stops the npm fallback through the alternative-lockfile guard. The globs are now reached by classic yarn.lock and bun.lockb alone. Across sca-npm-examples, fourteen of the nineteen skipped members are decided by a lockfile and five by the globs, all of them yarn classic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The package exported five package-manager identifiers that only its own resolvers use, so they advertised an API with no caller. They stay in names.py and leave the public surface. DENO_PACKAGE_MANAGER had no use anywhere once deno stopped resolving members, so it goes entirely. WorkspaceCoverage keeps its place: find_covering_workspace returns it, so it belongs in a signature a caller can read. The npm handler's is_project described only its first half, the guard against an alternative lockfile beside the manifest, and said nothing about the workspace coverage that the rest of this branch added. It now says both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amit-turgeman
force-pushed
the
CM-73389-skip-npm-workspace-member-lockfiles
branch
from
October 3, 2026 12:29
a706f78 to
44d3b87
Compare
omer-roth
approved these changes
Oct 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The CLI no longer generates a
package-lock.jsonfor an npm workspace member. npm installs a workspace from the root lockfile and ignores any lockfile inside a member, so the file we were generating was uploaded and never used.Worse than wasted work: generating it re-resolves the member's transitive ranges against the registry, so the scan sees versions the repository does not install. A committed root lockfile pinning
y18n 5.0.0/tough-cookie 2.3.0became5.0.8/2.3.4in the generated member lockfile — patched versions, hence no findings.When a member is skipped
A member is skipped when a lockfile above it already resolves its dependencies. The lockfile
itself is the authority on that, because it names the directories it resolved:
package-lock.json/npm-shrinkwrap.json(v2+)packages, excluding""andnode_modules/...pnpm-lock.yamlimportersyarn.lock(berry)resolutionof each<name>@workspace:<path>entryThe root manifest's
workspacesglobs are consulted only for lockfiles that cannot nametheir members: classic
yarn.lock, which is flat, and the binarybun.lockb. A v1package-lock.jsonis different again — workspaces arrived with npm 7 and lockfileVersion 2,so a v1 lockfile is simply not a workspace root and the globs must not rescue it.
This matches how
dependency-collectorresolves the same question, which derives memberattribution purely from lockfile content.
Correction to an earlier revision of this description
This description previously claimed that a
file:dependency target "is not resolved throughthe root lockfile, so it still needs its own", and used that to justify requiring a matching
workspacesglob. That claim was wrong. Checked against npm 11, afile:target and aworkspace member produce the same lockfile shape, and the target's ranges are pinned in the
root just like a member's:
Generating a second lockfile inside a
file:target can therefore only drift from the root.Such targets are now skipped like any other directory the root resolves.
Coverage
lockfileVersion2 and 3 — the only versions that can describe a workspacelockfileVersion1 has nopackagesmap and predates workspaces (npm 7), so it keeps generating as beforeworkspacesarray form and object form ({"packages": [...]})npm-shrinkwrap.json, which the backend already treats as equivalent*stops at a path separator,**does not, sopackages/*does not claimpackages/a/bTesting
30 unit tests,
ruff checkandruff format --checkclean on the pinned 0.15.20.Run against sca-benchmark, all 33 npm scenarios, comparing collected documents before and after:
npm/v3/18-combo-workspace-optionalnpm/v3/20-workspaces-object-formnpm/v3/09-peer-bundle-link-flagsandnpm/v3/10-file-directoryalso have nested manifests with no lockfile and are unchanged — they are the control, and10-file-directoryis the scenario that caught the original predicate being wrong.End to end: the resulting 3-document upload scanned against
dependency-collectoratmainyields 2 detections at the versions the committed lockfile pins.Jira
CM-73389
🤖 Generated with Claude Code