fix(cli): address James's review notes on stash env (#682)

- Note 2: treat an ABSENT access-key role as member (server default) rather
  than skipping the check — `(role ?? 'member')` — so the documented
  'verified on the response' guarantee doesn't depend on the field always
  being present.
- Note 3: only append the 'name already taken, use --name' hint on 400/409
  (where duplicates land), not every non-403 — a 500 no longer carries a
  misleading suggestion.
- Note 4 (defense-in-depth): reject any server-provided emitted value
  (CRN/region, client id, access key) that contains a control character,
  closing the dotenv line-injection surface the name guard already covered.
  clientKey is hex by construction.

Tests: absent-role → success; 500 omits the --name hint (keeps the
leftover-client note); a newline in the server region → refused, no
injected line on stdout. 27 env unit tests, suite 540/62 green.

Note 1 (CRN round-trip) needs no code change — the pre-merge live smoke
already inits a wasm-inline client with the emitted workspaceCrn (see PR
comment); replying there.

Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w

Author: coderdan

packages/cli/src/commands/env/__tests__/env.test.ts

@@ -338,6 +338,19 @@ describe('envCommand — failure modes', () => {
     expect(message).toContain('--name')
   })
 
+  it('does NOT append the --name hint to a 500 (only 400/409 are duplicates)', async () => {
+    stubSession()
+    stubFetch({
+      accessKey: new Response('internal error', { status: 500 }),
+    })
+
+    await expectExit(envCommand({ name: 'x' }), 1)
+    const message = lastError()
+    expect(message).not.toContain('already taken')
+    // The leftover-client note still appears — a client was minted.
+    expect(message).toContain("ZeroKMS client 'x'")
+  })
+
   it('maps a request timeout to a clear request_timeout error', async () => {
     stubSession()
     vi.stubGlobal(
@@ -433,6 +446,39 @@ describe('envCommand — response validation (nothing minted may print undefined
     // The over-privileged secret itself must not be printed anywhere.
     expect(stdout()).not.toContain('CSAKTid.secret')
   })
+
+  it('treats an absent role as member (server default) and succeeds', async () => {
+    stubSession()
+    stubFetch({
+      // No `role` field — the check must assume member, not skip.
+      accessKey: Response.json(
+        { accessKey: 'CSAKTid.secret' },
+        { status: 201 },
+      ),
+    })
+
+    await envCommand({ name: 'x' })
+    expect(stdout()).toContain('CS_CLIENT_ACCESS_KEY=CSAKTid.secret')
+  })
+
+  it('refuses to emit a server value containing a control character', async () => {
+    stubSession()
+    stubFetch({
+      // A newline in the region would break out of the CRN's dotenv line.
+      workspaces: Response.json([
+        { id: 'WS123', region: 'us-east-1.aws\nCS_INJECTED=evil' },
+      ]),
+    })
+
+    await expectExit(envCommand({ name: 'x', json: true }), 1)
+    const event = JSON.parse(logSpy.mock.calls.at(-1)?.[0] as string)
+    expect(event).toMatchObject({
+      status: 'error',
+      code: 'unexpected_response',
+    })
+    expect(String(event.message)).toContain('workspaceCrn')
+    expect(stdout()).not.toContain('CS_INJECTED')
+  })
 })
 
 describe('envCommand — --write', () => {

packages/cli/src/commands/env/index.ts

@@ -446,30 +446,53 @@ async function mintCredentials(keyName: string): Promise<MintedCredentials> {
       'create the access key',
       keyResponse,
     )
-    throw new MintError(
-      base.code,
-      `${base.message} If the name is already taken, rerun with a different --name.${leftover}`,
-    )
+    // A duplicate name comes back as 400 (BadRequest) or 409 (Conflict); only
+    // then is "pick a different name" the right suggestion — a 500 shouldn't
+    // carry it.
+    const dupHint =
+      keyResponse.status === 400 || keyResponse.status === 409
+        ? ' If the name is already taken, rerun with a different --name.'
+        : ''
+    throw new MintError(base.code, `${base.message}${dupHint}${leftover}`)
   }
   const accessKeyBody = parsed(
     accessKeySchema,
     await keyResponse.json(),
     'access key',
   )
-  if (accessKeyBody.role && accessKeyBody.role.toLowerCase() !== 'member') {
+  // The request pinned `member` and the server owns the default, so an ABSENT
+  // role means "server default (member)" — treat it as member rather than
+  // skipping the check, so the documented "verified on the response" guarantee
+  // doesn't quietly depend on the field always being present.
+  const returnedRole = (accessKeyBody.role ?? 'member').toLowerCase()
+  if (returnedRole !== 'member') {
     throw new MintError(
       'unexpected_role',
       `CTS returned a '${accessKeyBody.role}' access key where member was requested — refusing to emit it. Revoke '${keyName}' in the dashboard.`,
     )
   }
 
-  return {
+  const creds: MintedCredentials = {
     keyName,
     workspaceCrn,
     clientId: client.id,
     clientKey,
     accessKey: accessKeyBody.accessKey,
   }
+  // Defense-in-depth: the name is already control-char-guarded, but the
+  // server-provided values (CRN region, client id, access key) land on their
+  // own dotenv lines too. A control char — especially a newline — could break
+  // out of its line and inject another. Very low risk (trusted server), fully
+  // closed here. clientKey is hex by construction, so it can't carry one.
+  for (const [field, value] of Object.entries(creds)) {
+    if (CONTROL_CHARS.test(value)) {
+      throw new MintError(
+        'unexpected_response',
+        `The service returned a ${field} containing a control character — refusing to emit it. Check for a newer CLI release.`,
+      )
+    }
+  }
+  return creds
 }
 
 function trimTrailingSlash(url: string): string {