fix(config): user-agent properly shows ci

The way we were flattening user-agent back into itself meant that any
values that were dependent on other config items would never be seen,
since we have to re-flatten the item for each one it depends on.

We also weren't re-flattening the user-agent when setting workspaces or
workspace, which were things that affected the final result.

This does change the main config value of `user-agent` but not the
flattened one.  We are not using the main config value anywhere (which
is correct).

PR-URL: https://github.com/npm/cli/pull/3754
Credit: @wraithgar
Close: #3754
Reviewed-by: @nlf

5 files changed, 85 insertions, 9 deletions

diff --git a/lib/utils/config/definitions.js b/lib/utils/config/definitions.js
index 092e0fc43..009f60a7b 100644
--- a/lib/utils/config/definitions.js
+++ b/lib/utils/config/definitions.js
@@ -2053,10 +2053,14 @@ define('user-agent', {
         .replace(/\{workspaces\}/gi, inWorkspaces)
         .replace(/\{ci\}/gi, ciName ? `ci/${ciName}` : '')
         .trim()
+
+    // We can't clobber the original or else subsequent flattening will fail
+    // (i.e. when we change the underlying config values)
+    // obj[key] = flatOptions.userAgent
+
     // user-agent is a unique kind of config item that gets set from a template
     // and ends up translated.  Because of this, the normal "should we set this
     // to process.env also doesn't work
-    obj[key] = flatOptions.userAgent
     process.env.npm_config_user_agent = flatOptions.userAgent
   },
 })
@@ -2140,6 +2144,9 @@ define('workspace', {
     a workspace which does not yet exist, to create the folder and set it
     up as a brand new workspace within the project.
   `,
+  flatten: (key, obj, flatOptions) => {
+    definitions['user-agent'].flatten('user-agent', obj, flatOptions)
+  },
 })
 
 define('workspaces', {
@@ -2151,6 +2158,9 @@ define('workspaces', {
     Enable running a command in the context of **all** the configured
     workspaces.
   `,
+  flatten: (key, obj, flatOptions) => {
+    definitions['user-agent'].flatten('user-agent', obj, flatOptions)
+  },
 })
 
 define('yes', {
diff --git a/smoke-tests/index.js b/smoke-tests/index.js
index 9235c8960..5e2d5e071 100644
--- a/smoke-tests/index.js
+++ b/smoke-tests/index.js
@@ -1,8 +1,9 @@
 const fs = require('fs')
 const { promisify } = require('util')
 const execAsync = promisify(require('child_process').exec)
-const { resolve } = require('path')
+const { join, resolve } = require('path')
 const t = require('tap')
+const rimraf = promisify(require('rimraf'))
 
 const normalizePath = path => path.replace(/[A-Z]:/, '').replace(/\\/g, '/')
 const cwd = normalizePath(process.cwd())
@@ -47,6 +48,43 @@ const exec = async cmd => {
 const readFile = filename =>
   String(fs.readFileSync(resolve(localPrefix, filename)))
 
+// this test must come first, its package.json will be destroyed and the one
+// created in the next test (npm init) will create a new one that must be
+// present for later tests
+t.test('npm install sends correct user-agent', async t => {
+  const pkgPath = join(localPrefix, 'package.json')
+  const pkgContent = JSON.stringify({
+    name: 'smoke-test-workspaces',
+    workspaces: ['packages/*'],
+  })
+  fs.writeFileSync(pkgPath, pkgContent, { encoding: 'utf8' })
+
+  const wsRoot = join(localPrefix, 'packages')
+  fs.mkdirSync(wsRoot)
+
+  const wsPath = join(wsRoot, 'foo')
+  fs.mkdirSync(wsPath)
+
+  const wsPkgPath = join(wsPath, 'package.json')
+  const wsContent = JSON.stringify({
+    name: 'foo',
+  })
+  fs.writeFileSync(wsPkgPath, wsContent, { encoding: 'utf8' })
+  t.teardown(async () => {
+    await rimraf(`${localPrefix}/*`)
+  })
+
+  const cmd = `${npmBin} install fail_reflect_user_agent`
+  await t.rejects(exec(cmd), {
+    stderr: /workspaces\/false/,
+  }, 'workspaces/false is present in output')
+
+  const wsCmd = `${npmBin} install fail_reflect_user_agent --workspaces`
+  await t.rejects(exec(wsCmd), {
+    stderr: /workspaces\/true/,
+  }, 'workspaces/true is present in output')
+})
+
 t.test('npm init', async t => {
   const cmd = `${npmBin} init -y`
   const cmdRes = await exec(cmd)
diff --git a/smoke-tests/server.js b/smoke-tests/server.js
index b864114af..e0ac0c94e 100644
--- a/smoke-tests/server.js
+++ b/smoke-tests/server.js
@@ -1,5 +1,5 @@
 /* istanbul ignore file */
-const {join, dirname} = require('path')
+const {join, dirname, basename} = require('path')
 const {existsSync, readFileSync, writeFileSync} = require('fs')
 const PORT = 12345 + (+process.env.TAP_CHILD_ID || 0)
 const http = require('http')
@@ -150,6 +150,12 @@ const startServer = () => new Promise((res, rej) => {
     }
 
     const f = join(__dirname, 'content', join('/', req.url.replace(/@/, '').replace(/%2f/i, '/')))
+    // a magic package that causes us to return an error that will be logged
+    if (basename(f) === 'fail_reflect_user_agent') {
+      res.setHeader('npm-notice', req.headers['user-agent'])
+      res.writeHead(404)
+      return res.end()
+    }
     const isCorgi = req.headers.accept.includes('application/vnd.npm.install-v1+json')
     const file = f + (
       isCorgi && existsSync(`${f}.min.json`) ? '.min.json'
diff --git a/tap-snapshots/test/lib/config.js.test.cjs b/tap-snapshots/test/lib/config.js.test.cjs
index 817f3c173..b21b75cd2 100644
--- a/tap-snapshots/test/lib/config.js.test.cjs
+++ b/tap-snapshots/test/lib/config.js.test.cjs
@@ -146,7 +146,7 @@ exports[`test/lib/config.js TAP config list --json > output matches snapshot 1`]
   "unicode": false,
   "update-notifier": true,
   "usage": false,
-  "user-agent": "npm/{NPM-VERSION} node/{NODE-VERSION} {PLATFORM} {ARCH} workspaces/false",
+  "user-agent": "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}",
   "version": false,
   "versions": false,
   "viewer": "{VIEWER}",
@@ -296,7 +296,7 @@ umask = 0
 unicode = false 
 update-notifier = true 
 usage = false 
-user-agent = "npm/{NPM-VERSION} node/{NODE-VERSION} {PLATFORM} {ARCH} workspaces/false" 
+user-agent = "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}" 
 ; userconfig = "{HOME}/.npmrc" ; overridden by cli
 version = false 
 versions = false 
diff --git a/test/lib/utils/config/definitions.js b/test/lib/utils/config/definitions.js
index 65193020d..88993303b 100644
--- a/test/lib/utils/config/definitions.js
+++ b/test/lib/utils/config/definitions.js
@@ -747,7 +747,7 @@ t.test('user-agent', t => {
   definitions['user-agent'].flatten('user-agent', obj, flat)
   t.equal(flat.userAgent, expectNoCI)
   t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
-  t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
+  t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')
 
   obj['ci-name'] = 'foo'
   obj['user-agent'] = definitions['user-agent'].default
@@ -755,7 +755,7 @@ t.test('user-agent', t => {
   definitions['user-agent'].flatten('user-agent', obj, flat)
   t.equal(flat.userAgent, expectCI)
   t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
-  t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
+  t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')
 
   delete obj['ci-name']
   obj.workspaces = true
@@ -764,7 +764,7 @@ t.test('user-agent', t => {
   definitions['user-agent'].flatten('user-agent', obj, flat)
   t.equal(flat.userAgent, expectWorkspaces)
   t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
-  t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
+  t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')
 
   delete obj.workspaces
   obj.workspace = ['foo']
@@ -772,7 +772,7 @@ t.test('user-agent', t => {
   definitions['user-agent'].flatten('user-agent', obj, flat)
   t.equal(flat.userAgent, expectWorkspaces)
   t.equal(process.env.npm_config_user_agent, flat.userAgent, 'npm_user_config environment is set')
-  t.equal(obj['user-agent'], flat.userAgent, 'config user-agent template is translated')
+  t.not(obj['user-agent'], flat.userAgent, 'config user-agent template is not translated')
   t.end()
 })
 
@@ -853,3 +853,25 @@ t.test('package-lock-only', t => {
   t.strictSame(flat, { packageLock: false, packageLockOnly: false })
   t.end()
 })
+
+t.test('workspaces', t => {
+  const obj = {
+    workspaces: true,
+    'user-agent': definitions['user-agent'].default,
+  }
+  const flat = {}
+  definitions.workspaces.flatten('workspaces', obj, flat)
+  t.match(flat.userAgent, /workspaces\/true/)
+  t.end()
+})
+
+t.test('workspace', t => {
+  const obj = {
+    workspace: ['workspace-a'],
+    'user-agent': definitions['user-agent'].default,
+  }
+  const flat = {}
+  definitions.workspace.flatten('workspaces', obj, flat)
+  t.match(flat.userAgent, /workspaces\/true/)
+  t.end()
+})