Fix: ncm report — wrong coordinates, npm v7+ lockfile & audit compatibility, expanded license coverage#253
Fix: ncm report — wrong coordinates, npm v7+ lockfile & audit compatibility, expanded license coverage#253
Conversation
# 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 shared with #1) | `lib/ncm-analyze-tree.js` | | 4 | "we would need to investigate if our vulnDB has all the vulns" | Now meaningfully testable | see below | 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`](https://www.npmjs.com/package/dependency-tree) — a **source-code import analyzer** — instead of [`universal-module-tree`](https://www.npmjs.com/package/universal-module-tree), which walks the real npm dependency graph (`package-lock.json` → `yarn.lock` → `node_modules`). Three 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": ```js 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: ```js 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. ## Fix ### `lib/ncm-analyze-tree.js` — rewritten (550 → 131 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. The new file gets a tree from `universalModuleTree(dir)`, walks it keeping dedup + `paths` info, and POSTs `{ name, version }` pairs to the GraphQL endpoint — same shape that already worked in `commands/details.js`. ### `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`: ```js if (version === null || version === undefined) continue; ``` - Removed unused `includedCount` / `skippedCount` counters and the dead `getLicenseScore` helper. ### `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`. ### 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`: ```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: ```sh NCM_TOKEN=6c044113-c485-40cb-915b-cbc9d3a26730 \ NCM_API=http://localhost:3000 \ ncm report --dir=/path/to/some/project ``` --- ## Issue 4: vulnDB coverage investigation Previously impossible to do meaningfully — the payload was garbage (issue 1) and incomplete (issue 3). Now that real coordinates for the full transitive tree are sent, a direct diff is valid: ```sh cd /path/to/project npm install # ensure lockfile + node_modules npm audit --json > /tmp/npm-audit.json # npm's view NCM_TOKEN=… NCM_API=http://localhost:3000 \ ncm report --long --json > /tmp/ncm-report.json # ncm's view ``` Compare the `{name, version}` sets: - advisories in `npm-audit` but not `ncm-report` → real gaps in the vulnDB, feed back to the data team; - advisories in `ncm-report` but not `npm-audit` → extra NCM coverage (or a false positive to review). --- ## Files changed - `lib/ncm-analyze-tree.js` — rewritten (550 → 131 lines). - `commands/report.js` — `0.0.0` coercion and dead code removed. - `package.json`, `package-lock.json` — `dependency-tree` removed.
WalkthroughReplaces custom dependency-tree construction with universal-module-tree and a package-lock fast-path, removes version placeholder coercion and several bookkeeping counters, adds npm audit v7+ vulnerability extraction, and formats missing package versions as "N/A" in listings. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (commands/report)
participant Analyzer as Analyzer (lib/ncm-analyze-tree)
participant UMT as UMT (universal-module-tree)
participant Lock as Lockfile (package-lock.json)
participant Audit as npm-audit JSON
participant Reporter as Reporter (commands/report / pkgScores)
CLI->>Analyzer: read packages from dir
Analyzer->>Lock: inspect package-lock.json
alt lockJson.packages present
Lock-->>Analyzer: return name/version set (fast-path)
else
Analyzer->>UMT: universalModuleTree(dir)
UMT-->>Analyzer: module tree (children)
Analyzer->>Analyzer: build package set from tree.children
end
CLI->>Audit: fetch npm audit JSON
Audit-->>Reporter: return advisories OR vulnerabilities
Reporter->>Reporter: build versionByName map from NCM data
Reporter->>Reporter: exclude packages with missing versions
Reporter->>Reporter: append deduplicated vulnerability pkgScores
Reporter-->>CLI: produce final report
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
# 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 shared with #1) | `lib/ncm-analyze-tree.js` | | 4 | "we would need to investigate if our vulnDB has all the vulns" | Now meaningfully testable | see below | 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`](https://www.npmjs.com/package/dependency-tree) — a **source-code import analyzer** — instead of [`universal-module-tree`](https://www.npmjs.com/package/universal-module-tree), which walks the real npm dependency graph (`package-lock.json` → `yarn.lock` → `node_modules`). Three 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": ```js 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: ```js 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. ## Fix ### `lib/ncm-analyze-tree.js` — rewritten (550 → 131 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. The new file gets a tree from `universalModuleTree(dir)`, walks it keeping dedup + `paths` info, and POSTs `{ name, version }` pairs to the GraphQL endpoint — same shape that already worked in `commands/details.js`. ### `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`: ```js if (version === null || version === undefined) continue; ``` - Removed unused `includedCount` / `skippedCount` counters and the dead `getLicenseScore` helper. ### `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`. ### 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`: ```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: ```sh NCM_TOKEN=6c044113-c485-40cb-915b-cbc9d3a26730 \ NCM_API=http://localhost:3000 \ ncm report --dir=/path/to/some/project ``` --- ## Issue 4: vulnDB coverage investigation Previously impossible to do meaningfully — the payload was garbage (issue 1) and incomplete (issue 3). Now that real coordinates for the full transitive tree are sent, a direct diff is valid: ```sh cd /path/to/project npm install # ensure lockfile + node_modules npm audit --json > /tmp/npm-audit.json # npm's view NCM_TOKEN=… NCM_API=http://localhost:3000 \ ncm report --long --json > /tmp/ncm-report.json # ncm's view ``` Compare the `{name, version}` sets: - advisories in `npm-audit` but not `ncm-report` → real gaps in the vulnDB, feed back to the data team; - advisories in `ncm-report` but not `npm-audit` → extra NCM coverage (or a false positive to review). --- ## Files changed - `lib/ncm-analyze-tree.js` — rewritten (550 → 131 lines). - `commands/report.js` — `0.0.0` coercion and dead code removed. - `package.json`, `package-lock.json` — `dependency-tree` removed.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/ncm-analyze-tree.js (3)
74-74: Missing error handling foruniversalModuleTreecall.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 optionalintegrity/resolvedfields or malformed entries more defensively.The fast path reads
package-lock.jsonand extracts packages. While it correctly skips entries withoutnameorversion, there's no validation thatpkgDatais an object. IflockJson.packagescontains malformed entries (e.g.,nullvalues),pkgData.versioncould 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
fetchDatathrows for one page in a batch,Promise.allwill reject immediately but other concurrent requests continue executing. Their results are lost, and errors may be unhandled. Consider usingPromise.allSettledif 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
📒 Files selected for processing (3)
commands/report.jslib/ncm-analyze-tree.jslib/report/util.js
🚧 Files skipped from review as they are similar to previous changes (1)
- commands/report.js
# 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 shared with #1) | `lib/ncm-analyze-tree.js` | | 4 | "we would need to investigate if our vulnDB has all the vulns" | Now meaningfully testable | see below | 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`](https://www.npmjs.com/package/dependency-tree) — a **source-code import analyzer** — instead of [`universal-module-tree`](https://www.npmjs.com/package/universal-module-tree), which walks the real npm dependency graph (`package-lock.json` → `yarn.lock` → `node_modules`). Three 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": ```js 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: ```js 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. ## Fix ### `lib/ncm-analyze-tree.js` — rewritten (550 → 131 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. The new file gets a tree from `universalModuleTree(dir)`, walks it keeping dedup + `paths` info, and POSTs `{ name, version }` pairs to the GraphQL endpoint — same shape that already worked in `commands/details.js`. ### `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`: ```js if (version === null || version === undefined) continue; ``` - Removed unused `includedCount` / `skippedCount` counters and the dead `getLicenseScore` helper. ### `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`. ### 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`: ```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: ```sh NCM_TOKEN=6c044113-c485-40cb-915b-cbc9d3a26730 \ NCM_API=http://localhost:3000 \ ncm report --dir=/path/to/some/project ``` --- ## Issue 4: vulnDB coverage investigation Previously impossible to do meaningfully — the payload was garbage (issue 1) and incomplete (issue 3). Now that real coordinates for the full transitive tree are sent, a direct diff is valid: ```sh cd /path/to/project npm install # ensure lockfile + node_modules npm audit --json > /tmp/npm-audit.json # npm's view NCM_TOKEN=… NCM_API=http://localhost:3000 \ ncm report --long --json > /tmp/ncm-report.json # ncm's view ``` Compare the `{name, version}` sets: - advisories in `npm-audit` but not `ncm-report` → real gaps in the vulnDB, feed back to the data team; - advisories in `ncm-report` but not `npm-audit` → extra NCM coverage (or a false positive to review). --- ## Files changed - `lib/ncm-analyze-tree.js` — rewritten (550 → 131 lines). - `commands/report.js` — `0.0.0` coercion and dead code removed. - `package.json`, `package-lock.json` — `dependency-tree` removed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/lib/mock-packages.js (1)
6044-6587: Move the SPDX catalog out of the shared mock package fixture.This block is useful coverage, but adding it to the global
mock-packagesexport now changes unrelated report totals, ordering, and snapshots across the whole suite. The header comment already hints at the intent to avoid that, but thesepass: falseentries do affect counts. A dedicated fixture for certification/license rendering tests would keep the report tests much less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/lib/mock-packages.js` around lines 6044 - 6587, The SPDX license entries were added directly into the shared mock-packages export (the large SPDX catalog block in test/lib/mock-packages.js), which changes global report totals and breaks unrelated snapshots; extract that entire catalog into a dedicated test fixture (e.g., a new certification/license fixture module) and remove the SPDX entries from the shared mock-packages export so only tests that require certification/license rendering import the new fixture; update any tests that relied on those specific sample names (e.g., 'apache-2-0-sample', 'lgpl-3-0-sample', 'agpl-3-0-sample', etc.) to import the new fixture instead of relying on the global mock-packages list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/lib/mock-packages.js`:
- Around line 6044-6587: The SPDX license entries were added directly into the
shared mock-packages export (the large SPDX catalog block in
test/lib/mock-packages.js), which changes global report totals and breaks
unrelated snapshots; extract that entire catalog into a dedicated test fixture
(e.g., a new certification/license fixture module) and remove the SPDX entries
from the shared mock-packages export so only tests that require
certification/license rendering import the new fixture; update any tests that
relied on those specific sample names (e.g., 'apache-2-0-sample',
'lgpl-3-0-sample', 'agpl-3-0-sample', etc.) to import the new fixture instead of
relying on the global mock-packages list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca584347-49f3-45cf-b9be-a1b9df7e5f84
⛔ Files ignored due to path filters (2)
tap-snapshots/test/report.js.snapis excluded by!**/*.snaptest/fixtures/mock-project/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
commands/report.jstap-snapshots/test/report.js.mdtest/fixtures/mock-project/package.jsontest/lib/mock-packages.jstest/report.js
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/mock-project/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- commands/report.js
# 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 shared with #1) | `lib/ncm-analyze-tree.js` | | 4 | "we would need to investigate if our vulnDB has all the vulns" | Now meaningfully testable | see below | 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`](https://www.npmjs.com/package/dependency-tree) — a **source-code import analyzer** — instead of [`universal-module-tree`](https://www.npmjs.com/package/universal-module-tree), which walks the real npm dependency graph (`package-lock.json` → `yarn.lock` → `node_modules`). Three 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": ```js 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: ```js 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. ## Fix ### `lib/ncm-analyze-tree.js` — rewritten (550 → 131 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. The new file gets a tree from `universalModuleTree(dir)`, walks it keeping dedup + `paths` info, and POSTs `{ name, version }` pairs to the GraphQL endpoint — same shape that already worked in `commands/details.js`. ### `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`: ```js if (version === null || version === undefined) continue; ``` - Removed unused `includedCount` / `skippedCount` counters and the dead `getLicenseScore` helper. ### `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`. ### 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`: ```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: ```sh NCM_TOKEN=6c044113-c485-40cb-915b-cbc9d3a26730 \ NCM_API=http://localhost:3000 \ ncm report --dir=/path/to/some/project ``` --- ## Issue 4: vulnDB coverage investigation Previously impossible to do meaningfully — the payload was garbage (issue 1) and incomplete (issue 3). Now that real coordinates for the full transitive tree are sent, a direct diff is valid: ```sh cd /path/to/project npm install # ensure lockfile + node_modules npm audit --json > /tmp/npm-audit.json # npm's view NCM_TOKEN=… NCM_API=http://localhost:3000 \ ncm report --long --json > /tmp/ncm-report.json # ncm's view ``` Compare the `{name, version}` sets: - advisories in `npm-audit` but not `ncm-report` → real gaps in the vulnDB, feed back to the data team; - advisories in `ncm-report` but not `npm-audit` → extra NCM coverage (or a false positive to review). --- ## Files changed - `lib/ncm-analyze-tree.js` — rewritten (550 → 131 lines). - `commands/report.js` — `0.0.0` coercion and dead code removed. - `package.json`, `package-lock.json` — `dependency-tree` removed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/lib/mock-packages.js (1)
6049-6674: Consider a small factory + data table for SPDX sample mocks.This block is very repetitive; generating entries from a compact descriptor list would reduce drift/typos and make future SPDX additions safer.
Refactor sketch
+function makeLicenseSample (name, spdx, pass) { + return { + name, + version: '1.0.0', + published: true, + publishedAt: '2023-01-01T00:00:00.000Z', + maintainers: [], + keywords: [], + scores: [{ + group: 'compliance', + name: 'license', + pass, + severity: pass ? 'NONE' : 'MEDIUM', + title: `This package version license is ${pass ? 'acceptable' : 'unacceptable'}: '${spdx}'`, + data: { spdx } + }] + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/lib/mock-packages.js` around lines 6049 - 6674, The repeated SPDX sample objects should be generated from a small factory and descriptor table instead of duplicated literals: add a descriptor array (e.g. spdxDescriptors = [{name: 'apache-2-0-sample', spdx: 'Apache-2.0', pass: true}, ...]) and implement a factory function like createSpdxSample(descriptor) that returns the object shape used in this file (fields: name, version, published, publishedAt, maintainers, keywords, scores with data.spdx and pass/severity/title logic), then replace the hard-coded blocks by mapping spdxDescriptors.map(createSpdxSample); ensure titles match existing format and adjust any references that iterate this mock list accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/lib/mock-packages.js`:
- Around line 6044-6048: Update the misleading block comment that begins
"Additional license coverage for the certification flow" so it accurately
reflects that the sample entries include both passing and failing license
compliance cases (many entries have pass: false) rather than stating they are
"only a passing `license` compliance score"; locate the SPDX samples comment
block in test/lib/mock-packages.js (the "Additional license coverage for the
certification flow" section) and change the wording to indicate the dataset
contains mixed pass/fail license compliance examples used to exercise both
passing and failing report paths.
---
Nitpick comments:
In `@test/lib/mock-packages.js`:
- Around line 6049-6674: The repeated SPDX sample objects should be generated
from a small factory and descriptor table instead of duplicated literals: add a
descriptor array (e.g. spdxDescriptors = [{name: 'apache-2-0-sample', spdx:
'Apache-2.0', pass: true}, ...]) and implement a factory function like
createSpdxSample(descriptor) that returns the object shape used in this file
(fields: name, version, published, publishedAt, maintainers, keywords, scores
with data.spdx and pass/severity/title logic), then replace the hard-coded
blocks by mapping spdxDescriptors.map(createSpdxSample); ensure titles match
existing format and adjust any references that iterate this mock list
accordingly.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a72c672-fb4e-4c85-85bd-931d8f0a252a
📒 Files selected for processing (1)
test/lib/mock-packages.js
| /* --- Additional license coverage for the certification flow. */ | ||
| /* Each entry exists to demonstrate that a specific SPDX identifier flows */ | ||
| /* through the analyze → report pipeline. Scores are intentionally minimal — */ | ||
| /* only a passing `license` compliance score — so adding them does not skew */ | ||
| /* severity counts in report snapshots. */ |
There was a problem hiding this comment.
Correct the misleading comment above SPDX samples.
Line 6047 says entries are “only a passing license compliance score,” but many entries below are intentionally failing (pass: false). Please update the comment to reflect actual data behavior.
Suggested comment fix
-/* --- Additional license coverage for the certification flow. */
-/* Each entry exists to demonstrate that a specific SPDX identifier flows */
-/* through the analyze → report pipeline. Scores are intentionally minimal — */
-/* only a passing `license` compliance score — so adding them does not skew */
-/* severity counts in report snapshots. */
+/* --- Additional license coverage for the certification flow. */
+/* Each entry exists to demonstrate that a specific SPDX identifier flows */
+/* through the analyze → report pipeline. Scores are intentionally minimal — */
+/* only a `license` compliance score (pass/fail depending on policy). */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* --- Additional license coverage for the certification flow. */ | |
| /* Each entry exists to demonstrate that a specific SPDX identifier flows */ | |
| /* through the analyze → report pipeline. Scores are intentionally minimal — */ | |
| /* only a passing `license` compliance score — so adding them does not skew */ | |
| /* severity counts in report snapshots. */ | |
| /* --- Additional license coverage for the certification flow. */ | |
| /* Each entry exists to demonstrate that a specific SPDX identifier flows */ | |
| /* through the analyze → report pipeline. Scores are intentionally minimal — */ | |
| /* only a `license` compliance score (pass/fail depending on policy). */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/lib/mock-packages.js` around lines 6044 - 6048, Update the misleading
block comment that begins "Additional license coverage for the certification
flow" so it accurately reflects that the sample entries include both passing and
failing license compliance cases (many entries have pass: false) rather than
stating they are "only a passing `license` compliance score"; locate the SPDX
samples comment block in test/lib/mock-packages.js (the "Additional license
coverage for the certification flow" section) and change the wording to indicate
the dataset contains mixed pass/fail license compliance examples used to
exercise both passing and failing report paths.
Fix: ncm report — wrong coordinates, npm v7+ lockfile & audit compatibility, expanded license coverage
Summary
lib/ncm-analyze-tree.js,commands/report.jsNCM_TOKEN=… NCM_API=… ncm"with no success"lib/ncm-analyze-tree.js,commands/report.jsncm reportcrashes withCannot read properties of undefined (reading '@actions/core')on npm v7+lib/ncm-analyze-tree.jscommands/report.jsTests: 24 / 24 pass (
npm run test-only).Root cause (issues 1 & 3)
lib/ncm-analyze-tree.jshad been rewritten to usedependency-tree— a source-code import analyzer — instead ofuniversal-module-tree, which walks the real npm dependency graph (package-lock.json→yarn.lock→node_modules).Four concrete defects produced the payloads the user saw:
Filename-as-package-name with placeholder version.
dependency-treereturns resolved file paths. The replacement code turned each path into a "package":So
jsonwebtoken.js→jwt @ 0.0.0,index.js→index @ 0.0.0. That is the exactjwt version 0.0.0entry the user saw going out.Range-string fallback also produced
0.0.0. The auxiliarygetNpmDependencieshelper stripped^/~and fell back to'0.0.0'for non-semver specifiers ("latest","file:…","workspace:*", etc.).Missing transitive deps.
dependency-treeonly surfaces files the entry point actuallyrequire()s. The full transitive graph was never walked — the direct mechanical cause of the diff vsnpm audit.commands/report.jsmasked the fallout. Anynull/undefinedversionfrom the NCM service was coerced to'0.0.0'and still included in the rendered report.Root cause (issues 5 & 6) — npm v7+ compatibility
Issue 5:
universal-module-treeonly handlespackage-lock.jsonv1/v2, which has a top-leveldependenciesfield. npm v7+ generates v3 lockfiles with only apackagesfield. CallinguniversalModuleTree(dir)on a v3 lockfile throwsCannot read properties of undefined (reading '@actions/core').Issue 6:
npm audit --jsonchanged output format in npm v7+ fromauditReportVersion: 1(advisories) toauditReportVersion: 2(vulnerabilities). The code only checkednpmAuditJson.advisories, silently ignoring all audit results on npm v7+.Fix
lib/ncm-analyze-tree.js— rewritten (550 → 95 lines)Reverted to
universal-module-tree.Removed:
dependency-treeinvocation, entry-point guessing logic, filename-as-package-name logic,getNpmDependencies, duplicatereadPackagesFromPackageJson, dead counters.npm v7+ lockfile fix (issue 5): Reads
package-lock.jsonfirst. If v3 (no .dependenciesfield), builds the package set directly fromlock.packages, bypassinguniversalModuleTreeentirely:commands/report.jsRemoved
effectiveVersion = '0.0.0'coercion. Packages withversion === nullare now skipped:Removed unused
includedCount/skippedCountcounters and deadgetLicenseScorehelper.npm audit v7+ fix (issue 6): Handles
auditReportVersion: 2(vulnerabilities) alongside the existing v6 (advisories) handler. Versions are resolved from the NCM API result set; packages already reported by NCM are skipped to avoid duplicates:lib/report/util.jspkg.version != null ? String(pkg.version) : 'N/A'— handles audit entries where version may be absent.test/report.js+test/fixtures/AGPL-3.0-only,Python-2.0,Ruby,Sleepycat,UPL-1.0,Vim,W3C,X11,Zlib, and more.36 → 80 packages checked,2 → 27 noncompliant modules found, top-5 now dominated by license-failing packages.package.json/package-lock.jsondependency-tree.universal-module-treeis now the single source of truth for the dependency graph.Verification
Correct coordinates go out
npm v7+ (lockfileVersion 3) works end-to-end
Before: crash —
Cannot read properties of undefined (reading '@actions/core').After:
npm audit diff is now 0
Tested on
ncm-cliitself (677 packages, lockfileVersion 3):Tests: 24 / 24 pass
Issue 2: bare
ncm"with no success"ncmwith no subcommand shows help and exits 0 — same asgit/npm. No API request is ever made. To actually run the analyzer against a local API:Issue 4: vulnDB coverage — investigation done and fixed
The diff between
npm auditandncm reportrisk counts is now 0The root causes of the previously reported diff were code bugs (issues 1–3 and 6), now fixed.
Files changed
lib/ncm-analyze-tree.js— rewritten (550 → 95 lines); npm v7+ lockfile v3 support added.commands/report.js—0.0.0coercion removed; npm audit v7+ format support added.lib/report/util.js— null-safe version display.test/report.js— updated assertions for expanded fixture set.test/fixtures/— 25+ new SPDX license samples added.package.json,package-lock.json—dependency-treeremoved.Summary by CodeRabbit
New Features
Bug Fixes
Improvements