Merge remote-tracking branch 'origin' into oidc-default-provenance

Author: reggi

CONTRIBUTING.md

@@ -13,13 +13,13 @@ When submitting a new bug report, please first [search](https://github.com/npm/c
 **1. Clone this repository...**
 
 ```bash
-$ git clone git@github.com:npm/cli.git npm
+git clone git@github.com:npm/cli.git npm
 ```
 
 **2. Navigate into project & install development-specific dependencies...**
 
 ```bash
-$ cd ./npm && node ./scripts/resetdeps.js
+cd ./npm && node ./scripts/resetdeps.js
 ```
 
 **3. Write some code &/or add some tests...**
@@ -30,7 +30,7 @@ $ cd ./npm && node ./scripts/resetdeps.js
 
 **4. Run tests & ensure they pass...**
 ```
-$ node . run test
+node . run test
 ```
 
 **5. Open a [Pull Request](https://github.com/npm/cli/pulls) for your work & become the newest contributor to `npm`! 🎉**
@@ -61,9 +61,9 @@ node . exec
 ```
 
 For example instead of:
-```bash 
+```bash
 npm exec -- <package>
-```  
+```
 Use:
 ```bash
 node . exec -- <package>

docs/lib/content/configuring-npm/install.md

@@ -69,10 +69,3 @@ installers:
 
 Or see [this page](https://nodejs.org/en/download/package-manager/) to
 install npm for Linux in the way many Linux developers prefer.
-
-#### Less-common operating systems
-
-For more information on installing Node.js on a variety of operating
-systems, see [this page][pkg-mgr].
-
-[pkg-mgr]: https://nodejs.org/en/download/package-manager/

tap-snapshots/test/lib/docs.js.test.cjs

@@ -1290,7 +1290,8 @@ a semver. Like the \`rc\` in \`1.2.0-rc.8\`.
 
 #### \`progress\`
 
-* Default: \`true\` unless running in a known CI system
+* Default: \`true\` when not in CI and both stderr and stdout are TTYs and not
+  in a dumb terminal
 * Type: Boolean
 
 When set to \`true\`, npm will display a progress bar during time intensive

workspaces/arborist/lib/audit-report.js

@@ -1,5 +1,4 @@
 // an object representing the set of vulnerabilities in a tree
-/* eslint camelcase: "off" */
 
 const localeCompare = require('@isaacs/string-locale-compare')('en')
 const npa = require('npm-package-arg')
@@ -8,16 +7,15 @@ const pickManifest = require('npm-pick-manifest')
 const Vuln = require('./vuln.js')
 const Calculator = require('@npmcli/metavuln-calculator')
 
-const _getReport = Symbol('getReport')
-const _fixAvailable = Symbol('fixAvailable')
-const _checkTopNode = Symbol('checkTopNode')
-const _init = Symbol('init')
-const _omit = Symbol('omit')
 const { log, time } = require('proc-log')
 
 const npmFetch = require('npm-registry-fetch')
 
 class AuditReport extends Map {
+  #omit
+  error = null
+  topVulns = new Map()
+
   static load (tree, opts) {
     return new AuditReport(tree, opts).run()
   }
@@ -91,22 +89,18 @@ class AuditReport extends Map {
 
   constructor (tree, opts = {}) {
     super()
-    const { omit } = opts
-    this[_omit] = new Set(omit || [])
-    this.topVulns = new Map()
-
+    this.#omit = new Set(opts.omit || [])
     this.calculator = new Calculator(opts)
-    this.error = null
     this.options = opts
     this.tree = tree
     this.filterSet = opts.filterSet
   }
 
   async run () {
-    this.report = await this[_getReport]()
+    this.report = await this.#getReport()
     log.silly('audit report', this.report)
     if (this.report) {
-      await this[_init]()
+      await this.#init()
     }
     return this
   }
@@ -116,7 +110,7 @@ class AuditReport extends Map {
     return !!(vuln && vuln.isVulnerable(node))
   }
 
-  async [_init] () {
+  async #init () {
     const timeEnd = time.start('auditReport:init')
 
     const promises = []
@@ -171,7 +165,15 @@ class AuditReport extends Map {
           vuln.nodes.add(node)
           for (const { from: dep, spec } of node.edgesIn) {
             if (dep.isTop && !vuln.topNodes.has(dep)) {
-              this[_checkTopNode](dep, vuln, spec)
+              vuln.fixAvailable = this.#fixAvailable(vuln, spec)
+              if (vuln.fixAvailable !== true) {
+                // now we know the top node is vulnerable, and cannot be
+                // upgraded out of the bad place without --force.  But, there's
+                // no need to add it to the actual vulns list, because nothing
+                // depends on root.
+                this.topVulns.set(vuln.name, vuln)
+                vuln.topNodes.add(dep)
+              }
             } else {
             // calculate a metavuln, if necessary
               const calc = this.calculator.calculate(dep.packageName, advisory)
@@ -214,33 +216,14 @@ class AuditReport extends Map {
     timeEnd()
   }
 
-  [_checkTopNode] (topNode, vuln, spec) {
-    vuln.fixAvailable = this[_fixAvailable](topNode, vuln, spec)
-
-    if (vuln.fixAvailable !== true) {
-      // now we know the top node is vulnerable, and cannot be
-      // upgraded out of the bad place without --force.  But, there's
-      // no need to add it to the actual vulns list, because nothing
-      // depends on root.
-      this.topVulns.set(vuln.name, vuln)
-      vuln.topNodes.add(topNode)
-    }
-  }
-
-  // check whether the top node is vulnerable.
-  // check whether we can get out of the bad place with --force, and if
-  // so, whether that update is SemVer Major
-  [_fixAvailable] (topNode, vuln, spec) {
-    // this will always be set to at least {name, versions:{}}
-    const paku = vuln.packument
-
+  // given the spec, see if there is a fix available at all, and note whether or not it's a semver major fix or not (i.e. will need --force)
+  #fixAvailable (vuln, spec) {
+    // TODO we return true, false, OR an object here. this is probably a bad pattern.
     if (!vuln.testSpec(spec)) {
       return true
     }
 
-    // similarly, even if we HAVE a packument, but we're looking for it
-    // somewhere other than the registry, and we got something vulnerable,
-    // then we're stuck with it.
+    // even if we HAVE a packument, if we're looking for it somewhere other than the registry and we have something vulnerable then we're stuck with it.
     const specObj = npa(spec)
     if (!specObj.registry) {
       return false
@@ -250,15 +233,13 @@ class AuditReport extends Map {
       spec = specObj.subSpec.rawSpec
     }
 
-    // We don't provide fixes for top nodes other than root, but we
-    // still check to see if the node is fixable with a different version,
-    // and if that is a semver major bump.
+    // we don't provide fixes for top nodes other than root, but we still check to see if the node is fixable with a different version, and note if that is a semver major bump.
     try {
       const {
         _isSemVerMajor: isSemVerMajor,
         version,
         name,
-      } = pickManifest(paku, spec, {
+      } = pickManifest(vuln.packument, spec, {
         ...this.options,
         before: null,
         avoid: vuln.range,
@@ -274,7 +255,7 @@ class AuditReport extends Map {
     throw new Error('do not call AuditReport.set() directly')
   }
 
-  async [_getReport] () {
+  async #getReport () {
     // if we're not auditing, just return false
     if (this.options.audit === false || this.options.offline === true || this.tree.inventory.size === 1) {
       return null
@@ -312,11 +293,17 @@ class AuditReport extends Map {
 
   // return true if we should audit this one
   shouldAudit (node) {
-    return !node.version ? false
-      : node.isRoot ? false
-      : this.filterSet && this.filterSet.size !== 0 && !this.filterSet.has(node) ? false
-      : this[_omit].size === 0 ? true
-      : !node.shouldOmit(this[_omit])
+    if (
+      !node.version ||
+      node.isRoot ||
+      (this.filterSet && this.filterSet?.size !== 0 && !this.filterSet?.has(node))
+    ) {
+      return false
+    }
+    if (this.#omit.size === 0) {
+      return true
+    }
+    return !node.shouldOmit(this.#omit)
   }
 
   prepareBulkData () {

workspaces/config/lib/definitions/definitions.js

@@ -1571,9 +1571,9 @@ const definitions = {
     },
   }),
   progress: new Definition('progress', {
-    default: !ciInfo.isCI,
+    default: !(ciInfo.isCI || !process.stderr.isTTY || !process.stdout.isTTY || process.env.TERM === 'dumb'),
     defaultDescription: `
-      \`true\` unless running in a known CI system
+      \`true\` when not in CI and both stderr and stdout are TTYs and not in a dumb terminal
     `,
     type: Boolean,
     description: `
@@ -1583,11 +1583,8 @@ const definitions = {
       Set to \`false\` to suppress the progress bar.
     `,
     flatten (key, obj, flatOptions) {
-      flatOptions.progress = !obj.progress ? false
-        // progress is only written to stderr but we disable it unless stdout is a tty
-        // also. This prevents the progress from appearing when piping output to another
-        // command which doesn't break anything, but does look very odd to users.
-        : !!process.stderr.isTTY && !!process.stdout.isTTY && process.env.TERM !== 'dumb'
+      // Only show progress if explicitly enabled AND we have proper TTY environment
+      flatOptions.progress = !!obj.progress && !!process.stderr.isTTY && !!process.stdout.isTTY && process.env.TERM !== 'dumb'
     },
   }),
   provenance: new Definition('provenance', {

workspaces/config/test/definitions/definitions.js

@@ -402,6 +402,7 @@ t.test('progress', t => {
 
   const flat = {}
 
+  // Test flatten function behavior
   mockDefs().progress.flatten('progress', {}, flat)
   t.strictSame(flat, { progress: false })
 
@@ -417,6 +418,59 @@ t.test('progress', t => {
   mockDefs().progress.flatten('progress', { progress: true }, flat)
   t.strictSame(flat, { progress: false })
 
+  // Ensures consistency between default and flatOptions behavior
+  t.test('default value consistency', t => {
+    // Test case 1: Both TTYs, normal terminal, not CI
+    setEnv({ tty: true, term: 'xterm' })
+    const def1 = mockDefs({ 'ci-info': { isCI: false, name: null } }).progress
+    t.equal(def1.default, true, 'default should be true when both TTYs, normal terminal, and not CI')
+
+    // Test case 2: No TTYs, not CI
+    setEnv({ tty: false, term: 'xterm' })
+    const def2 = mockDefs({ 'ci-info': { isCI: false, name: null } }).progress
+    t.equal(def2.default, false, 'default should be false when no TTYs')
+
+    // Test case 3: Both TTYs but dumb terminal, not CI
+    setEnv({ tty: true, term: 'dumb' })
+    const def3 = mockDefs({ 'ci-info': { isCI: false, name: null } }).progress
+    t.equal(def3.default, false, 'default should be false in dumb terminal')
+
+    // Test case 4: Mixed TTY states, not CI
+    mockGlobals(t, {
+      'process.stderr.isTTY': true,
+      'process.stdout.isTTY': false,
+      'process.env.TERM': 'xterm',
+    })
+    const def4 = mockDefs({ 'ci-info': { isCI: false, name: null } }).progress
+    t.equal(def4.default, false, 'default should be false when only one TTY')
+
+    // Test case 5: Good TTY environment but in CI
+    setEnv({ tty: true, term: 'xterm' })
+    const def5 = mockDefs({ 'ci-info': { isCI: true, name: 'github-actions' } }).progress
+    t.equal(def5.default, false, 'default should be false in CI even with good TTY environment')
+
+    t.end()
+  })
+
+  // Test that flatten behavior is independent of CI detection
+  t.test('flatten function ignores CI detection', t => {
+    const flatObj = {}
+
+    // Test that CI doesn't affect flatten behavior when user explicitly enables
+    setEnv({ tty: true, term: 'xterm' })
+    const defsCI = mockDefs({ 'ci-info': { isCI: true, name: 'github-actions' } })
+    defsCI.progress.flatten('progress', { progress: true }, flatObj)
+    t.equal(flatObj.progress, true, 'flatten should enable progress in CI if user explicitly sets true and TTY is available')
+
+    // Test that non-CI doesn't guarantee flatten success if TTY is bad
+    setEnv({ tty: false, term: 'xterm' })
+    const defsNoCI = mockDefs({ 'ci-info': { isCI: false, name: null } })
+    defsNoCI.progress.flatten('progress', { progress: true }, flatObj)
+    t.equal(flatObj.progress, false, 'flatten should disable progress outside CI if TTY is not available')
+
+    t.end()
+  })
+
   t.end()
 })