fix(sim-setup): stop doctor --fix from turning ${VAR} references into literals

packages/sim-setup/src/checks.test.ts

@@ -1,6 +1,21 @@
-import { describe, expect, it } from 'vitest'
-import { type CheckContext, runChecks } from './checks'
-import type { EnvFile, EnvTarget } from './env-files'
+import { mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import path from 'node:path'
+import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { fakeRoot } = vi.hoisted(() => {
+  const { mkdtempSync } = require('node:fs') as typeof import('node:fs')
+  const { tmpdir } = require('node:os') as typeof import('node:os')
+  const { join } = require('node:path') as typeof import('node:path')
+  return { fakeRoot: mkdtempSync(join(tmpdir(), 'sim-setup-checks-')) }
+})
+
+vi.mock('./context', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('./context')>()
+  return { ...actual, SETUP_CONTEXT: { kind: 'source', root: fakeRoot } }
+})
+
+import { type CheckContext, loadCheckContext, runChecks } from './checks'
+import { type EnvFile, type EnvTarget, parseEnv } from './env-files'
 
 function envFile(target: EnvTarget, values: Record<string, string> = {}): EnvFile {
   return {
@@ -9,6 +24,7 @@ function envFile(target: EnvTarget, values: Record<string, string> = {}): EnvFil
     exists: target === 'root',
     content: '',
     vars: new Map(Object.entries(values)),
+    verbatim: new Map(Object.entries(values)),
   }
 }
 
@@ -65,3 +81,83 @@ describe('setup coherence checks', () => {
     })
   })
 })
+
+describe('doctor --fix on env files', () => {
+  const files = {
+    sim: path.join(fakeRoot, 'apps/sim/.env'),
+    realtime: path.join(fakeRoot, 'apps/realtime/.env'),
+    db: path.join(fakeRoot, 'packages/db/.env'),
+  }
+  const referencingDsn = `postgresql://\${DB_USER}@localhost/sim`
+
+  function writeFiles(contents: Record<keyof typeof files, string>) {
+    for (const [target, content] of Object.entries(contents)) {
+      const file = files[target as keyof typeof files]
+      mkdirSync(path.dirname(file), { recursive: true })
+      writeFileSync(file, content)
+    }
+  }
+
+  async function fixConsistency() {
+    const findings = await runChecks(loadCheckContext(false), ['consistency'])
+    for (const finding of findings) finding.autofix?.()
+    return findings.find((finding) => finding.message.startsWith('DATABASE_URL differs'))
+  }
+
+  beforeEach(() => rmSync(fakeRoot, { recursive: true, force: true }))
+  afterAll(() => rmSync(fakeRoot, { recursive: true, force: true }))
+
+  it.each([
+    ['a different URL', 'DATABASE_URL=postgresql://other@localhost/sim\n'],
+    [
+      'the reference escaped into a literal',
+      `DATABASE_URL="postgresql://\\\${DB_USER}@localhost/sim"\n`,
+    ],
+  ])('reports a $VAR DATABASE_URL against %s and leaves it for a hand copy', async (_, dbFile) => {
+    writeFiles({
+      sim: `DB_USER=alice\nDATABASE_URL=${referencingDsn}\n`,
+      realtime: '',
+      db: dbFile,
+    })
+
+    const finding = await fixConsistency()
+
+    expect(finding).toMatchObject({ status: 'fail', autofix: undefined })
+    expect(readFileSync(files.db, 'utf8')).toBe(dbFile)
+  })
+
+  it.each([
+    ['missing', 'DATABASE_URL=postgresql://localhost/sim\n'],
+    ['a placeholder', 'BETTER_AUTH_SECRET=your_auth_secret_here\n'],
+  ])(
+    'never generates a shared secret it cannot copy when realtime has it %s',
+    async (_, realtime) => {
+      const secret = `BETTER_AUTH_SECRET=\${AUTH_SECRET_BASE}0123456789abcdef0123456789abcdef`
+      writeFiles({ sim: `${secret}\n`, realtime, db: '' })
+
+      for (const finding of await runChecks(loadCheckContext(false), ['schema']))
+        finding.autofix?.()
+
+      const after = parseEnv(readFileSync(files.realtime, 'utf8')).get('BETTER_AUTH_SECRET')
+      expect(after).toBe(parseEnv(realtime).get('BETTER_AUTH_SECRET'))
+    }
+  )
+
+  it('reports a value it cannot write instead of aborting the other fixes', async () => {
+    const unwritable = 'DATABASE_URL="postgresql://u:p\\$x\\y@localhost/sim"\n'
+    const dbFile = 'DATABASE_URL=postgresql://other@localhost/sim\n'
+    writeFiles({ sim: unwritable, realtime: '', db: dbFile })
+
+    const finding = await fixConsistency()
+
+    expect(finding).toMatchObject({ status: 'fail', autofix: undefined })
+    expect(readFileSync(files.db, 'utf8')).toBe(dbFile)
+  })
+
+  it('accepts a $VAR DATABASE_URL written the same way in both files', async () => {
+    const line = `DATABASE_URL=${referencingDsn}\n`
+    writeFiles({ sim: `DB_USER=alice\n${line}`, realtime: '', db: line })
+
+    expect(await fixConsistency()).toBeUndefined()
+  })
+})

packages/sim-setup/src/checks.ts

@@ -17,6 +17,7 @@ import { portOpen } from './detect'
 import {
   type EnvFile,
   type EnvTarget,
+  envValueRefusal,
   generateSecret,
   isPlaceholder,
   isTruthy,
@@ -30,6 +31,35 @@ import {
 import { httpHealth, pgProbe, redisPing } from './probes'
 import { FLAG_TWINS, LOGIN_PROVIDERS } from './twins'
 
+/**
+ * Whether `key` is written with an unescaped `$`, a reference Next, Bun, and
+ * Compose expand against the file it sits in. Copied to another file it can
+ * resolve differently, and copied as a literal it stops expanding, so doctor
+ * leaves such values for the user to mirror by hand.
+ */
+function holdsReference(file: EnvFile, key: string): boolean {
+  return /(^|[^\\])\$/.test(file.verbatim.get(key) ?? '')
+}
+
+/**
+ * Whether `key` holds the same thing in both files. Parsed values hide the one
+ * difference that matters for `$`: `${VAR}` expands while `"\${VAR}"` does not.
+ */
+function sameEnvValue(a: EnvFile, b: EnvFile, key: string): boolean {
+  if (a.vars.get(key) !== b.vars.get(key)) return false
+  if (!holdsReference(a, key) && !holdsReference(b, key)) return true
+  return a.verbatim.get(key) === b.verbatim.get(key)
+}
+
+/** Why doctor cannot copy `key` out of `file` for the user, or `undefined` when it can. */
+function mirrorBlocker(file: EnvFile, key: string, value: string): string | undefined {
+  if (holdsReference(file, key)) {
+    return 'it contains a $ reference, so copy it and the variables it uses by hand'
+  }
+  const refusal = envValueRefusal(value)
+  return refusal && `${refusal}, so copy it by hand`
+}
+
 export type CheckGroup = 'files' | 'schema' | 'consistency' | 'coherence' | 'live'
 export type CheckStatus = 'pass' | 'warn' | 'fail' | 'skip'
 
@@ -144,7 +174,9 @@ function checkFiles(ctx: CheckContext): Finding[] {
               const value = ctx.env.sim.vars.get(key)
               // Skip placeholders so seeding never copies an .env.example stub
               // into the new file (matches autofixForMissing).
-              if (value && !isPlaceholder(value)) values[key] = value
+              if (value && !isPlaceholder(value) && !mirrorBlocker(ctx.env.sim, key, value)) {
+                values[key] = value
+              }
             }
             writeEnvValues(target, values)
           }
@@ -166,6 +198,8 @@ function autofixForMissing(
     simValue &&
     !isPlaceholder(simValue)
   ) {
+    // A generated secret would differ from the app's, so a value doctor can't copy gets no fix.
+    if (mirrorBlocker(ctx.env.sim, key, simValue)) return undefined
     return () => writeEnvValues(target, { [key]: simValue })
   }
   if (MIN_32_KEYS.has(key)) {
@@ -195,16 +229,16 @@ function checkSchema(ctx: CheckContext): Finding[] {
         continue
       }
       if (isPlaceholder(value)) {
+        // Same fix as a missing key: a shared secret is mirrored from the app, never regenerated.
+        const autofix = MIN_32_KEYS.has(key) ? autofixForMissing(ctx, target, key) : undefined
         findings.push({
           group: 'schema',
           status: production ? 'fail' : 'warn',
           message: `${rel(file)}: ${key} still has the .env.example placeholder`,
-          fix: MIN_32_KEYS.has(key)
-            ? 'doctor --fix generates a real value'
+          fix: autofix
+            ? 'doctor --fix sets a real value'
             : `replace the placeholder in ${rel(file)}`,
-          autofix: MIN_32_KEYS.has(key)
-            ? () => writeEnvValues(target, { [key]: generateSecret() })
-            : undefined,
+          autofix,
         })
         continue
       }
@@ -262,28 +296,34 @@ function checkConsistency(ctx: CheckContext): Finding[] {
       const simValue = sim.vars.get(key)
       const realtimeValue = realtime.vars.get(key)
       if (!simValue || !realtimeValue) continue
-      if (simValue !== realtimeValue) {
+      if (!sameEnvValue(sim, realtime, key)) {
+        const blocker = mirrorBlocker(sim, key, simValue)
         findings.push({
           group: 'consistency',
           status: 'fail',
           message: `${key} differs between apps/sim/.env and apps/realtime/.env`,
-          fix: 'doctor --fix mirrors the apps/sim/.env value',
-          autofix: () => writeEnvValues('realtime', { [key]: simValue }),
+          fix: blocker
+            ? `mirror the apps/sim/.env value: ${blocker}`
+            : 'doctor --fix mirrors the apps/sim/.env value',
+          autofix: blocker ? undefined : () => writeEnvValues('realtime', { [key]: simValue }),
         })
       }
     }
   }
   if (sim.exists && db.exists) {
     const simDsn = sim.vars.get('DATABASE_URL')
     const dbDsn = db.vars.get('DATABASE_URL')
-    if (simDsn && dbDsn && simDsn !== dbDsn) {
+    if (simDsn && dbDsn && !sameEnvValue(sim, db, 'DATABASE_URL')) {
+      const blocker = mirrorBlocker(sim, 'DATABASE_URL', simDsn)
       findings.push({
         group: 'consistency',
         status: 'fail',
         message:
           'DATABASE_URL differs between apps/sim/.env and packages/db/.env — migrations would hit a different database',
-        fix: 'doctor --fix mirrors the apps/sim/.env value',
-        autofix: () => writeEnvValues('db', { DATABASE_URL: simDsn }),
+        fix: blocker
+          ? `mirror the apps/sim/.env value: ${blocker}`
+          : 'doctor --fix mirrors the apps/sim/.env value',
+        autofix: blocker ? undefined : () => writeEnvValues('db', { DATABASE_URL: simDsn }),
       })
     }
   }

packages/sim-setup/src/env-files.ts

@@ -58,10 +58,22 @@ export interface EnvFile {
   exists: boolean
   content: string
   vars: Map<string, string>
+  /** Each value as written in the file, quotes and escapes included. */
+  verbatim: Map<string, string>
 }
 
 const LINE_RE = /^\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*=\s*(.*)$/
 
+/** The value as written: a quoted value up to its closing quote, else up to an inline comment. */
+function verbatimValue(raw: string): string {
+  const trimmed = raw.trim()
+  if (trimmed.startsWith('"') || trimmed.startsWith("'")) {
+    const end = trimmed.indexOf(trimmed[0], 1)
+    return end === -1 ? trimmed : trimmed.slice(0, end + 1)
+  }
+  return trimmed.replace(/\s+#.*$/, '').trim()
+}
+
 function parseValue(raw: string): string {
   const trimmed = raw.trim()
   if (trimmed.startsWith('"') || trimmed.startsWith("'")) {
@@ -86,7 +98,12 @@ export function readEnvFile(target: EnvTarget): EnvFile {
   const filePath = ENV_PATHS[target]
   const exists = existsSync(filePath)
   const content = exists ? readFileSync(filePath, 'utf8') : ''
-  return { target, path: filePath, exists, content, vars: parseEnv(content) }
+  const verbatim = new Map<string, string>()
+  for (const line of content.split('\n')) {
+    const match = LINE_RE.exec(line)
+    if (match) verbatim.set(match[1], verbatimValue(match[2]))
+  }
+  return { target, path: filePath, exists, content, vars: parseEnv(content), verbatim }
 }
 
 /**
@@ -96,24 +113,34 @@ export function readEnvFile(target: EnvTarget): EnvFile {
  * expand `$NAME` even inside single quotes, and the one spelling all four read as a
  * literal `$` is `\$` inside double quotes.
  */
-function formatEnvValue(key: string, value: string): string {
-  if (/[\r\n]/.test(value)) {
-    throw new Error(`${key} cannot be written to an environment file: it contains a line break`)
-  }
+function spellEnvValue(value: string): { text: string } | { refusal: string } {
+  if (/[\r\n]/.test(value)) return { refusal: 'it contains a line break' }
   if (value.includes('$')) {
     if (/["\\`]/.test(value)) {
-      throw new Error(
-        `${key} cannot be written to an environment file: it mixes $ with a double quote, backslash, or backtick`
-      )
+      return { refusal: 'it mixes $ with a double quote, backslash, or backtick' }
     }
-    return `"${value.replaceAll('$', '\\$')}"`
+    return { text: `"${value.replaceAll('$', '\\$')}"` }
+  }
+  if (value === value.trim() && !value.includes('#') && !/^["'`]/.test(value)) {
+    return { text: value }
+  }
+  if (!value.includes("'")) return { text: `'${value}'` }
+  if (!/["\\`]/.test(value)) return { text: `"${value}"` }
+  return { refusal: 'it mixes quotes with a # or edge whitespace' }
+}
+
+/** Why `upsertEnv` cannot write `value` so every loader reads it back unchanged, if it cannot. */
+export function envValueRefusal(value: string): string | undefined {
+  const spelling = spellEnvValue(value)
+  return 'refusal' in spelling ? spelling.refusal : undefined
+}
+
+function formatEnvValue(key: string, value: string): string {
+  const spelling = spellEnvValue(value)
+  if ('refusal' in spelling) {
+    throw new Error(`${key} cannot be written to an environment file: ${spelling.refusal}`)
   }
-  if (value === value.trim() && !value.includes('#') && !/^["'`]/.test(value)) return value
-  if (!value.includes("'")) return `'${value}'`
-  if (!/["\\`]/.test(value)) return `"${value}"`
-  throw new Error(
-    `${key} cannot be written to an environment file: it mixes quotes with a # or edge whitespace`
-  )
+  return spelling.text
 }
 
 /**

packages/sim-setup/src/lifecycle.ts

@@ -595,9 +595,14 @@ async function reset(install: Install | null): Promise<void> {
   // With the daemon down nothing managed is visible, so a reset would archive the
   // env files while the volumes they unlock survive.
   if (install?.kind !== 'k8s' && executableExists('docker') && !dockerReachable()) {
+    // With no install detected, a Kubernetes install whose context isn't current looks the same.
+    const hints = [`start Docker, then re-run ${theme.command('npx sim-setup reset')}`]
+    if (!install) {
+      hints.push(`if Sim runs on Kubernetes, switch kubectl to its cluster's context and re-run`)
+    }
     throw new SetupError(
       "Docker isn't reachable, so the managed containers and volumes can't be removed. Your env files were left in place.",
-      [`start Docker, then re-run ${theme.command('npx sim-setup reset')}`]
+      hints
     )
   }
   const ok = await p.confirm({