Uh oh!
There was an error while loading. Please reload this page.
feat(dependencies): collect pnpm packages - #657
Conversation
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
📝 WalkthroughWalkthroughThe dependency collector now supports Node.js dependencies from pnpm lockfiles. It resolves npm license metadata, reports Node.js packages, updates tests, and documents the new Changespnpm Dependency Collection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant scanTree
participant pnpmLockfile
participant npmRegistry
participant dependencyReport
scanTree->>pnpmLockfile: parse pnpm dependencies
pnpmLockfile-->>scanTree: return package names and versions
scanTree->>npmRegistry: request license metadata
npmRegistry-->>scanTree: return license or unresolved result
scanTree->>dependencyReport: generate Node rows and counts
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tools/collect-dependencies/node_deps.go (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
node_deps.goto follow the tools filename rule.Rename this new file to
node-deps.go. Update any filename-based tooling in the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/collect-dependencies/node_deps.go` at line 16, Rename the Go source file currently named node_deps.go to node-deps.go to comply with the tools filename convention, and update any filename-based tooling or references that depend on the old name.Source: Coding guidelines
tools/collect-dependencies/main_test.go (1)
176-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
link:dependency exclusion.The fixture verifies
file:exclusion but notlink:exclusion. Add a quoted package key such aslinked@link:../workspaceand assert thatparsePNPMLockexcludes it.As per coding guidelines, for changed tool behavior, add or update focused tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/collect-dependencies/main_test.go` around lines 176 - 184, In the lockfile fixture passed to writeTestFile, add a quoted package entry with a link: dependency (such as linked@link:../workspace with its resolution block) alongside the existing file:../workspace entry. Update the assertions that follow the parsePNPMLock call to verify that this new link: dependency is excluded from the parsed results, mirroring the coverage already in place for the file: exclusion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/collect-dependencies/node_deps.go`:
- Around line 39-45: Update parsePNPMLock to return the underlying read or YAML
parsing error instead of an empty dependency set, then propagate that error
through scanTree and run so failures stop report generation rather than
producing incomplete results.
- Around line 91-92: Update httpGetString and npmLicense to accept and propagate
context.Context, create requests with http.NewRequestWithContext, and inject W3C
trace context into outbound headers. On request failures, mark the active span
with error=true and otel.status_code=ERROR, and add the required OpenTelemetry
dependency while updating all affected callers.
---
Nitpick comments:
In `@tools/collect-dependencies/main_test.go`:
- Around line 176-184: In the lockfile fixture passed to writeTestFile, add a
quoted package entry with a link: dependency (such as linked@link:../workspace
with its resolution block) alongside the existing file:../workspace entry.
Update the assertions that follow the parsePNPMLock call to verify that this new
link: dependency is excluded from the parsed results, mirroring the coverage
already in place for the file: exclusion.
In `@tools/collect-dependencies/node_deps.go`:
- Line 16: Rename the Go source file currently named node_deps.go to
node-deps.go to comply with the tools filename convention, and update any
filename-based tooling or references that depend on the old name.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d056f9f8-f55e-43ee-93e8-d5520bb7457f
📒 Files selected for processing (7)
dependencies.mdtools/collect-dependencies/README.mdtools/collect-dependencies/common.gotools/collect-dependencies/main.gotools/collect-dependencies/main_test.gotools/collect-dependencies/node_deps.gotools/collect-dependencies/scan.go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/collect-dependencies/node-deps.go (2)
113-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHandle the array form of the npm
licensefield.Older npm packages publish
licenseorlicensesas an array of objects, for example[{"type":"MIT","url":"..."}].npmLicenseValuereturns""for that shape, so those packages get the "unknown or unreachable" label even though the registry returned a license. Add an[]anycase.♻️ Proposed handling for array license values
func npmLicenseValue(value any) string { switch license := value.(type) { case string: return strings.TrimSpace(license) case map[string]any: if name, ok := license["type"].(string); ok { return strings.TrimSpace(name) } + case []any:+ names := []string{}+ for _, item := range license {+ if name := npmLicenseValue(item); name != "" {+ names = append(names, name)+ }+ }+ return strings.Join(names, " OR ") } return "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/collect-dependencies/node-deps.go` around lines 113 - 123, Update npmLicenseValue to handle []any license values by inspecting the array entries for a license object and extracting its string "type" field with the same trimming behavior as the existing map case; preserve the current string, map, and empty-result behavior.
125-140: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider bounded concurrency for the npm lookups.
buildNodeRowsperforms one sequential HTTPS request per package. A pnpm lockfile in a monorepo can hold thousands of entries, so a full run can take several minutes. A worker pool with a small fixed limit (for example 8) plus a mutex aroundcachewould reduce wall-clock time substantially. This matches the existing pattern for the other languages, so it can also be deferred to a follow-up that changes all collectors together.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/collect-dependencies/node-deps.go` around lines 125 - 140, Replace the sequential npmLicense calls within the buildNodeRows loop with a bounded worker pool pattern (e.g., 8 concurrent workers) that processes the dependency keys concurrently. Add a mutex to protect access to the cache map since it will now be read and written by multiple goroutines simultaneously. Preserve the existing logic for building each dependencyRow and populating the license field, ensuring all requests complete before the function returns the final rows slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tools/collect-dependencies/node-deps.go`:
- Around line 113-123: Update npmLicenseValue to handle []any license values by
inspecting the array entries for a license object and extracting its string
"type" field with the same trimming behavior as the existing map case; preserve
the current string, map, and empty-result behavior.
- Around line 125-140: Replace the sequential npmLicense calls within the
buildNodeRows loop with a bounded worker pool pattern (e.g., 8 concurrent
workers) that processes the dependency keys concurrently. Add a mutex to protect
access to the cache map since it will now be read and written by multiple
goroutines simultaneously. Preserve the existing logic for building each
dependencyRow and populating the license field, ensuring all requests complete
before the function returns the final rows slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8424fc92-7292-436a-af5b-d523f53ee14e
📒 Files selected for processing (1)
tools/collect-dependencies/node-deps.go
Uh oh!
There was an error while loading. Please reload this page.
The import landed source-only: the subtree kept its upstream MODULE.bazel and sat in .bazelignore, so nothing in this repository built, tested or linted its 185 TypeScript and 29 Go files. Excluding the upstream .gitlab-ci.yml had also removed the six quality gates it used to run, replacing them with none. This finishes the migration so the code is actually verified here. check-nested-modules now reports `awaiting migration: 0` again, so the import no longer works against the consolidation. The merge, in the order the failures surfaced: - Lifted aspect_rules_js and rules_nodejs plus the node and npm extensions into the root MODULE.bazel, repointed at //src/uis/nvcf-ui/ui:pnpm-lock.yaml. - Registered the backend in go.work.bazel and rewrote subtree-relative labels. - Dropped the subtree's MODULE.bazel, .bazelrc, rules/ and platforms/; the root already provides distroless_static, rules/oci and every Go dependency the backend imports. - Lifted the subtree's .bazelignore entries into the root. aspect_rules_js refuses to evaluate npm_translate_lock unless every nested node_modules pnpm will create is ignored, so this is required, not tidiness. - Ported go_web_oci_image into the root rules/oci. The root copy was not equivalent; it had no such rule. - Dropped that rule's repo_tag parameter. Its own doc said it existed because package_name() is empty at a repo root; at src/uis/nvcf-ui it is not, so the root's derived load tag applies and the workaround is obsolete. - Removed //platforms:sources and //rules/oci:sources from the container's source bundle. Those are root build infrastructure, not UI source. - Aligned the backend's controller-runtime from v0.24.1 to v0.22.5 to match the rest of the repository. A version-specific replace pins client-go to v0.34.2 while MVS was selecting controller-runtime v0.24.1 from this module, and 0.24 needs client-go APIs that 0.34 does not have. Merges main for the pnpm dependency collector added in #657. Without it the UI's npm tree never reached dependencies.md; the regenerated file now records 531 Node.js packages. Adds the nvcf-ui matrix row so the subtree builds and tests on its own changes. Verified locally: `bazel build //src/uis/nvcf-ui/...` covers 31 targets and `bazel test //src/uis/nvcf-ui/...` passes 5 of 5, including the frontend vitest suite. The controller-runtime downgrade is proven at compile and unit-test level only. The token-watcher is the component using it, and its tests do not exercise cache behaviour, so a reviewer from the UI team should confirm 0.22.5 is acceptable at runtime. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
The import landed source-only: the subtree kept its upstream MODULE.bazel and sat in .bazelignore, so nothing in this repository built, tested or linted its 185 TypeScript and 29 Go files. Excluding the upstream .gitlab-ci.yml had also removed the six quality gates it used to run, replacing them with none. This finishes the migration so the code is actually verified here. check-nested-modules now reports `awaiting migration: 0` again, so the import no longer works against the consolidation. The merge, in the order the failures surfaced: - Lifted aspect_rules_js and rules_nodejs plus the node and npm extensions into the root MODULE.bazel, repointed at //src/uis/nvcf-ui/ui:pnpm-lock.yaml. - Registered the backend in go.work.bazel and rewrote subtree-relative labels. - Dropped the subtree's MODULE.bazel, .bazelrc, rules/ and platforms/; the root already provides distroless_static, rules/oci and every Go dependency the backend imports. - Lifted the subtree's .bazelignore entries into the root. aspect_rules_js refuses to evaluate npm_translate_lock unless every nested node_modules pnpm will create is ignored, so this is required, not tidiness. - Ported go_web_oci_image into the root rules/oci. The root copy was not equivalent; it had no such rule. - Dropped that rule's repo_tag parameter. Its own doc said it existed because package_name() is empty at a repo root; at src/uis/nvcf-ui it is not, so the root's derived load tag applies and the workaround is obsolete. - Removed //platforms:sources and //rules/oci:sources from the container's source bundle. Those are root build infrastructure, not UI source. - Aligned the backend's controller-runtime from v0.24.1 to v0.22.5 to match the rest of the repository. A version-specific replace pins client-go to v0.34.2 while MVS was selecting controller-runtime v0.24.1 from this module, and 0.24 needs client-go APIs that 0.34 does not have. Merges main for the pnpm dependency collector added in #657. Without it the UI's npm tree never reached dependencies.md; the regenerated file now records 531 Node.js packages. Adds the nvcf-ui matrix row so the subtree builds and tests on its own changes. Verified locally: `bazel build //src/uis/nvcf-ui/...` covers 31 targets and `bazel test //src/uis/nvcf-ui/...` passes 5 of 5, including the frontend vitest suite. The controller-runtime downgrade is proven at compile and unit-test level only. The token-watcher is the component using it, and its tests do not exercise cache behaviour, so a reviewer from the UI team should confirm 0.22.5 is acceptable at runtime. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
TL;DR
Additional Details
COLLECT_DEPS_NO_NPM=1to disable npm lookups.For QA
go test -C ./tools/collect-dependencies .Issues
Closes#581
Checklist
Summary by CodeRabbit
New Features
Documentation