Fix: wrong package coordinates sent to NCM API

[JungMinu]

Fix: wrong package coordinates sent to NCM API

Summary

#	Report	Status	Where
1	"sending odd packages versions to the API like this one jwt version 0.0.0"	Fixed	lib/ncm-analyze-tree.js, commands/report.js
2	NCM_TOKEN=… NCM_API=… ncm "with no success"	Not a bug; documented below	—
3	"still a diff on npm audit and ncm report"	Fixed (root cause fixed + npm v7+ audit format)	lib/ncm-analyze-tree.js, commands/report.js
4	"we would need to investigate if our vulnDB has all the vulns"	Fixed — now diff is 0	see below
5	ncm report crashes with Cannot read properties of undefined (reading '@actions/core') on npm v7+	Fixed	lib/ncm-analyze-tree.js
6	npm audit vulnerabilities silently dropped on npm v7+	Fixed	commands/report.js

Tests: 24 / 24 pass (npm run test-only).

---

Root cause (issues 1 & 3)

lib/ncm-analyze-tree.js had been rewritten to use dependency-tree — a source-code import analyzer — instead of universal-module-tree, which walks the real npm dependency graph (package-lock.json → yarn.lock → node_modules).

Four concrete defects in that code produced the payloads the user saw:

1. Filename-as-package-name with placeholder version. dependency-tree returns resolved file paths. The replacement code turned each path into a "package":

   const name = path.basename(dep, path.extname(dep));
   const childNode = {
     data: { name, version: '0.0.0' }, // hard-coded
     children: []
   };

   So jsonwebtoken.js → jwt @ 0.0.0, index.js → index @ 0.0.0, and similar. That is the exact jwt version 0.0.0 entry the user saw going out.

2. Range-string fallback also produced 0.0.0. The auxiliary getNpmDependencies helper only read direct deps from package.json and did:

   const cleanVersion = version.replace(/[^\d.]/g, '') || version;

   For "latest", "file:…", "git+https://…", "workspace:*", etc. the strip leaves '' and the || fallback is still non-semver — so those deps were emitted as name @ 0.0.0 too.

3. Missing transitive deps. dependency-tree only surfaces files the entry point actually require()s, and the package.json fallback only listed direct dependencies. The full transitive graph was never walked. This is the direct mechanical cause of the diff vs npm audit.

4. commands/report.js had been patched to hide the fallout. Any null / undefined version returned by the NCM service was coerced to '0.0.0' and still included in the rendered report, so bogus rows showed up in the UI too.

Root cause (issues 5 & 6) — npm v7+ compatibility

Two additional breakages affecting projects using npm v7+ (lockfileVersion 3):

Issue 5: universal-module-tree only handles package-lock.json v1/v2 format, which has a top-level dependencies field. npm v7+ generates v3 lockfiles with only a packages field. Calling universalModuleTree(dir) on a v3 lockfile hits packageLock.dependencies['@actions/core'] where packageLock.dependencies is undefined, throwing Cannot read properties of undefined (reading '@actions/core').

Issue 6: npm audit --json changed its output format in npm v7+ from auditReportVersion: 1 (advisories field) to auditReportVersion: 2 (vulnerabilities field). The existing code only checked npmAuditJson.advisories, so all npm audit results were silently ignored on npm v7+, causing the report to show zero risks regardless of actual vulnerabilities.

---

Fix

lib/ncm-analyze-tree.js — rewritten (550 → 95 lines)

• Reverted to universal-module-tree (already in dependencies, and the same library commands/details.js uses).

• Removed, in full:

  • the dependency-tree invocation and entry-point discovery logic (guessing index.js, app.js, bin/*.js, …),
  • filename-as-package-name logic,
  • getNpmDependencies and the duplicate readPackagesFromPackageJson that stripped ^ / ~ and fell back to '0.0.0',
  • dead counters.

• npm v7+ lockfile fix (issue 5): Reads package-lock.json before calling universalModuleTree. If lockfileVersion >= 3 (no .dependencies field), builds the package set directly from lock.packages, bypassing universalModuleTree entirely:

  if (lockJson && !lockJson.dependencies && lockJson.packages) {
    for (const [pkgPath, pkgData] of Object.entries(lockJson.packages)) {
      if (!pkgPath) continue
      const parts = pkgPath.split('node_modules/')
      const name = parts[parts.length - 1]
      const version = pkgData.version
      if (!name || !version) continue
      set.add({ name, version, paths: [[]] })
    }
    return set
  }

commands/report.js

• Removed the effectiveVersion = '0.0.0' coercion. Packages the NCM service returns with version === null (unpublished / unknown) are now skipped instead of rendered as @ 0.0.0:

  if (version === null || version === undefined) continue;

• Removed unused includedCount / skippedCount counters and the dead getLicenseScore helper.

• npm audit v7+ fix (issue 6): Added handling for auditReportVersion: 2 format alongside the existing v6 handler. Versions are resolved from the NCM API result set:

  } else if (npmAuditJson.vulnerabilities) {
    // npm v7+ format
    for (const [name, vuln] of Object.entries(npmAuditJson.vulnerabilities)) {
      const { severity = 'none' } = vuln
      const maxSeverity = SEVERITY_RMAP_NPM.indexOf(severity.toUpperCase())
      pkgScores.push({ name, version: versionByName.get(name), ... })
    }
  }

lib/report/util.js

• Null-safe version display: pkg.version != null ? String(pkg.version) : 'N/A' to handle entries from npm audit where the version is not available in the audit output.

package.json / package-lock.json

• Removed the unused dependency-tree dependency (npm uninstall dependency-tree). universal-module-tree was already listed and is now the single source of truth for the dependency graph.

---

Verification

Empirical: correct coordinates go out

$ node -e "const umt=require('universal-module-tree');
  (async()=>{
    const t=await umt('./test/fixtures/mock-project');
    const f=umt.flatten(t);
    console.log('total:', f.length);
    console.log('zero-versions:', f.filter(p=>p.version==='0.0.0'));
    console.log('filename-like:', f.filter(p=>/\\.|\//.test(p.name)));
  })()"
total: 37
zero-versions: []
filename-like: []

No @ 0.0.0, no jwt-style filename names — just real installed packages from package-lock.json.

npm v7+ projects: ncm report now works end-to-end

Before: crash with Cannot read properties of undefined (reading '@actions/core').

After (on a project with lockfileVersion 3):

493 packages checked

  ! 1 critical risk
    8 high risk
    7 medium risk
    3 low risk

npm audit vulnerabilities are now correctly surfaced in the risk summary counts.

Tests: npm run test-only → 24 / 24 pass

Notable guards:

• report › report output matches snapshot — asserts "36 packages checked", which only holds when the transitive graph is walked correctly.
• report › report with poisoned project — asserts a mock package returned with version: null is skipped, not rendered as @ 0.0.0.
• All --filter=*, --compliance, --security, --long snapshot tests pass, plus details, whitelist, install, github-actions, and proxied suites.

---

Issue 2: bare ncm "with no success"

ncm with no subcommand shows help and exits 0 — same as git or npm without args. bin/ncm-cli.js:

let [command = 'help', ...subargs] = argv._
if (!Object.keys(commands).includes(command)) command = 'help'

No API request is ever made, which is why the debug session looked like "nothing happening". Verified post-fix: node bin/ncm-cli.js prints help and exits 0. To actually exercise the analyzer against the local API:

NCM_TOKEN=6c044113-c485-40cb-915b-cbc9d3a26730 \
NCM_API=http://localhost:3000 \
  ncm report --dir=/path/to/some/project

---

Issue 4: vulnDB coverage — investigation done and fixed

The root cause of the previously reported diff was twofold:

1. Wrong package coordinates being sent to the NCM API (issues 1–3) — fixed by reverting to universal-module-tree.
2. npm audit v7+ results being silently dropped (issue 6) — fixed by handling the vulnerabilities format.

---

Files changed

• lib/ncm-analyze-tree.js — rewritten (550 → 95 lines); added npm v7+ lockfile v3 support.
• commands/report.js — 0.0.0 coercion and dead code removed; added npm audit v7+ format support.
• lib/report/util.js — null-safe version display.
• package.json, package-lock.json — dependency-tree removed.

[coderabbitai]

Walkthrough

Refactors dependency-tree construction to use universal-module-tree, adds npm audit v7+ vulnerability handling, and removes unused counters and placeholder-version normalization in report generation and package listing.

Changes

Cohort / File(s)	Summary
Report Generation commands/report.js	Removed includedCount/skippedCount bookkeeping and effectiveVersion placeholder '0.0.0' normalization; skip packages with missing versions instead. Dropped getLicenseScore, simplified control flow, and added handling for npm audit v7+ vulnerabilities using a versionByName map to populate pkgScores.
Tree Analysis lib/ncm-analyze-tree.js	Replaced custom dependency-tree construction with direct universal-module-tree usage (readUniversalTree awaits universalModuleTree(dir) and walks tree.children). Added package-lock.json fast-path when lockJson.packages exists. Removed prior fallback implementations and simplified filterPkgs (dedupe, semver validation, predicate filtering).
Report Utilities lib/report/util.js	moduleList() now defensively formats versions: uses 'N/A' when pkg.version is null/undefined instead of stringifying those values.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as CLI (commands/report)
  participant Analyzer as Analyzer (ncm-analyze-tree)
  participant UMT as universal-module-tree
  participant Audit as npm audit JSON
  participant Reporter as pkgScores / report.js

  CLI->>Analyzer: request package read (dir)
  Analyzer->>UMT: universalModuleTree(dir)
  UMT-->>Analyzer: module tree (children)
  Analyzer->>Analyzer: build package set (name,version) / lockJson fast-path
  CLI->>Audit: obtain npm audit JSON
  Audit-->>Reporter: vulnerabilities array (v7+)
  Analyzer-->>Reporter: package set + versionByName map
  Reporter->>Reporter: filter out packages with missing versions
  Reporter->>Reporter: push pkgScores entries (including vulnerabilities)
  Reporter-->>CLI: final report output

Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through trees both new and old,
Replaced the paths with branches bold,
Counters gone, versions spared the trick,
Vulnerabilities mapped quick-click,
A cleaner hop — review, behold! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix: wrong package coordinates sent to NCM API' directly addresses the main issue: correcting incorrect package name/version pairs passed to the API, which is the core problem resolved by reverting to universal-module-tree and removing the dependency-tree approach.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-ncm-rev

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (3)

lib/ncm-analyze-tree.js (3)

74-74: Missing error handling for universalModuleTree call.

If universalModuleTree(dir) throws (e.g., directory doesn't exist, invalid package.json), the error propagates unhandled. Consider whether a fallback or more descriptive error message would improve the user experience.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/ncm-analyze-tree.js` at line 74, The call to universalModuleTree(dir) can
throw and is not caught; wrap the await universalModuleTree(dir) invocation in a
try/catch (in lib/ncm-analyze-tree.js around the const tree assignment), capture
the thrown error, and handle it by logging or rethrowing a new Error with
contextual information (include dir and the original error message) or returning
a sensible fallback (e.g., empty tree) depending on desired behavior; ensure the
rest of the function uses the caught result (tree) only after successful
resolution.

---

61-72: Consider handling optional integrity/resolved fields or malformed entries more defensively.

The fast path reads package-lock.json and extracts packages. While it correctly skips entries without name or version, there's no validation that pkgData is an object. If lockJson.packages contains malformed entries (e.g., null values), pkgData.version could throw.

🛡️ Proposed defensive check

   for (const [pkgPath, pkgData] of Object.entries(lockJson.packages)) {
     if (!pkgPath) continue
+    if (!pkgData || typeof pkgData !== 'object') continue
     const parts = pkgPath.split('node_modules/')
     const name = parts[parts.length - 1]
     const version = pkgData.version

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/ncm-analyze-tree.js` around lines 61 - 72, The fast-path loop over
lockJson.packages lacks defensive checks for malformed pkgData (it assumes
pkgData is an object and accesses pkgData.version); update the loop in
lib/ncm-analyze-tree.js that iterates over lockJson.packages to first ensure
pkgData is a non-null object (e.g., typeof pkgData === 'object' && pkgData !==
null), validate that pkgData.version is present and a string, and tolerate
optional fields like integrity/resolved; if checks fail, skip that entry instead
of accessing properties, then continue calling set.add({ name, version, paths:
[[]] }) only for validated entries.

---

26-32: Unhandled rejection if any page fetch fails.

If fetchData throws for one page in a batch, Promise.all will reject immediately but other concurrent requests continue executing. Their results are lost, and errors may be unhandled. Consider using Promise.allSettled if partial results are acceptable, or adding explicit error handling.

💡 Example with error handling

   for (const batch of batches) {
-    await Promise.all([...batch].map(async page => {
-      const fetchedData = await fetchData({ pkgs: page, token, url })
-      for (const datum of fetchedData) {
-        data.add(datum)
-      }
-    }))
+    const results = await Promise.allSettled([...batch].map(async page => {
+      return fetchData({ pkgs: page, token, url })
+    }))
+    for (const result of results) {
+      if (result.status === 'fulfilled') {
+        for (const datum of result.value) {
+          data.add(datum)
+        }
+      }
+      // Optionally log or collect rejected results
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/ncm-analyze-tree.js` around lines 26 - 32, The current batch concurrency
uses Promise.all([...batch].map(async page => { const fetchedData = await
fetchData({ pkgs: page, token, url }) ... })) which will reject immediately on
the first fetchData error and lose other results; change this to use
Promise.allSettled for each batch, iterate the settled results, add values from
fulfilled results into data (using data.add) and explicitly handle/rethrow or
log rejections from the rejected entries so errors are not left unhandled and
partial results are preserved.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@lib/ncm-analyze-tree.js`:
- Line 74: The call to universalModuleTree(dir) can throw and is not caught;
wrap the await universalModuleTree(dir) invocation in a try/catch (in
lib/ncm-analyze-tree.js around the const tree assignment), capture the thrown
error, and handle it by logging or rethrowing a new Error with contextual
information (include dir and the original error message) or returning a sensible
fallback (e.g., empty tree) depending on desired behavior; ensure the rest of
the function uses the caught result (tree) only after successful resolution.
- Around line 61-72: The fast-path loop over lockJson.packages lacks defensive
checks for malformed pkgData (it assumes pkgData is an object and accesses
pkgData.version); update the loop in lib/ncm-analyze-tree.js that iterates over
lockJson.packages to first ensure pkgData is a non-null object (e.g., typeof
pkgData === 'object' && pkgData !== null), validate that pkgData.version is
present and a string, and tolerate optional fields like integrity/resolved; if
checks fail, skip that entry instead of accessing properties, then continue
calling set.add({ name, version, paths: [[]] }) only for validated entries.
- Around line 26-32: The current batch concurrency uses
Promise.all([...batch].map(async page => { const fetchedData = await fetchData({
pkgs: page, token, url }) ... })) which will reject immediately on the first
fetchData error and lose other results; change this to use Promise.allSettled
for each batch, iterate the settled results, add values from fulfilled results
into data (using data.add) and explicitly handle/rethrow or log rejections from
the rejected entries so errors are not left unhandled and partial results are
preserved.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4ff009c5-b393-41a5-be05-bd53683368c9

📥 Commits

Reviewing files that changed from the base of the PR and between 83caf73 and 200acb5.

📒 Files selected for processing (3)

• commands/report.js
• lib/ncm-analyze-tree.js
• lib/report/util.js

🚧 Files skipped from review as they are similar to previous changes (1)

• commands/report.js