Merged
Conversation
All standalone js code that eng writes uses standard, as a policy.
Fishrock123
commented
Nov 17, 2018
| .options(options) | ||
| .help(false) | ||
| .argv No newline at end of file | ||
| yargs /* eslint-disable-line no-unused-expressions */ |
Contributor
Author
There was a problem hiding this comment.
idk how to fix this one, we don't normally run into this. I bet it's from the .argv at the end.
Contributor
There was a problem hiding this comment.
I'll dig a bit deeper into the yargs api, hopefully should provide some insight into resolution.
Fishrock123
commented
Nov 17, 2018
Fishrock123
commented
Nov 17, 2018
| function config (argv) { | ||
| let help = (argv['_'] && argv['_'][1] === 'help') || argv.help | ||
|
|
||
| let help = (argv['_'] && argv['_'][1] == 'help') || argv.help |
Contributor
Author
There was a problem hiding this comment.
Lots of these, == usually is best avoided as a mistake upstream could cause a false check.
see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness
Contributor
There was a problem hiding this comment.
Appreciate the clarification. I can see what you mean, w.r.t upstream mistakes.
Strict Equality Comparison all the way.
Fishrock123
commented
Nov 17, 2018
| } | ||
| const options = { | ||
| method: 'GET', | ||
| hostname: api, |
Contributor
Author
There was a problem hiding this comment.
I took a guess that this was what you meant. This code evidently didn't work since config isn't even defined in this file! 😉
Contributor
Author
|
Other notes:
|
JungMinu
added a commit
that referenced
this pull request
Apr 21, 2026
# 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.
JungMinu
added a commit
that referenced
this pull request
Apr 22, 2026
# 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.
JungMinu
added a commit
that referenced
this pull request
Apr 22, 2026
# 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.
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.
All standalone js code that eng writes uses standard, as a policy.
This keeps the code all looking the same, and avoids many common errors, some of which were caught here. Might as well do it sooner than later. :)
We can try getting this set up with something to automatically run this on ever PR, too.