fix(cli): review round 1 — flag lookup, terminal controls, download safety

## CLI flags silently dropped (Cursor, High)

Commander camelCases every multi-word flag, so `--min-duration-ms` is stored as
`minDurationMs`. `buildRequest` looked flags up by their own kebab name, found
nothing, and dropped the field — no error, it just never reached the API. That
was every multi-word flag on every generated command.

The unit tests passed because they fed flag values already keyed by flag name,
which is not what commander produces — they validated a fiction. Added
`build.test.ts`, which parses real argv through the built commands; three of its
assertions fail against the previous code. The old tests now use camelCase keys
with a comment saying why.

## Terminal control sequences (Greptile, P1 security)

`stripAnsi` matched only SGR (`ESC [ … m`), so a knowledge document, table cell,
or workflow name could carry OSC, non-SGR CSI, or `ESC c` through to an
interactive terminal — setting the window title, moving the cursor to overwrite
what was already printed, or resetting the terminal. Replaced with a `sanitize`
covering OSC (BEL- and ST-terminated), CSI, any ESC + printable, and the bare
C0/C1 range, keeping tab and newline. Applied where API values become display
text, so the colour the CLI adds afterwards still works.

## Downloads (Greptile, P1 ×2)

`createWriteStream` truncated silently, and the destination name usually comes
from the server's content-disposition rather than anything the caller typed —
so a download could irreversibly replace an unrelated local file. Now opens `wx`
and fails with a message naming `--force`, which was added for the deliberate
overwrite.

The stream's error listener was attached after the read loop finished, so an
EEXIST/EACCES/ENOSPC during writing was an unhandled 'error' event that took
down the process. It is now registered before the first write and raced against
the pump.

## Personal-key caption (Cursor, Low)

With "No workspace (personal key)" picked, the caption still promised a default
workspace the approval does not send. It now distinguishes no-pick from
picked-but-not-admin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EsThUPqZXjwuuyRjBbmVkj

Author: TheodoreSpeaks

apps/sim/app/cli/auth/cli-auth-view.tsx

@@ -121,7 +121,12 @@ export function CliAuthView() {
                   ? 'Could not load your workspaces. Connecting still works and issues a personal key; reload to pick a default workspace.'
                   : bindsToWorkspace
                     ? `Issues a key that can only reach ${chosen.name}.`
-                    : 'Issues a personal key tied to your account, defaulting to this workspace. Workspace-scoped keys need admin.'}
+                    : chosen
+                      ? 'Issues a personal key tied to your account, defaulting to this workspace. Workspace-scoped keys need admin.'
+                      : // No workspace picked, so none is sent and none becomes the
+                        // profile default — promising one here would describe a
+                        // grant that Connect is not about to make.
+                        'Issues a personal key tied to your account, with no default workspace.'}
             </p>
           </div>
         )}

packages/sim-cli/src/commands/auth.ts

@@ -25,14 +25,22 @@ import { printRecord } from '../output/render.js'
  * falls through to the user pasting it somewhere.
  */
 function openBrowser(url: string): void {
-  const command =
-    process.platform === 'darwin' ? 'open' : process.platform === 'win32' ? 'start' : 'xdg-open'
+  /**
+   * Windows needs `cmd /c start "" <url>`.
+   *
+   * `start` is a cmd builtin, so it needs a shell — but its first quoted
+   * argument is the *window title*, and node quotes the URL because of the `?`
+   * and `&` in the query. Passing the URL alone therefore opens a console
+   * titled with the handoff link and no browser at all. The empty `""` takes
+   * the title slot so the URL lands where it belongs.
+   */
+  const [command, args] =
+    process.platform === 'win32'
+      ? ['cmd', ['/c', 'start', '', url]]
+      : [process.platform === 'darwin' ? 'open' : 'xdg-open', [url]]
+
   try {
-    const child = spawn(command, [url], {
-      stdio: 'ignore',
-      detached: true,
-      shell: process.platform === 'win32',
-    })
+    const child = spawn(command, args, { stdio: 'ignore', detached: true })
     child.on('error', () => {})
     child.unref()
   } catch {}

packages/sim-cli/src/commands/hand-written.ts

@@ -6,7 +6,7 @@ import type { Command } from 'commander'
 import { clientFrom } from '../context.js'
 import type { QueryRowsResponse } from '../generated/v2-api.js'
 import { SimApiError } from '../http/client.js'
-import { type Column, printList, text } from '../output/render.js'
+import { type Column, printList, sanitize, text } from '../output/render.js'
 
 /**
  * Commands the generated runtime cannot produce.
@@ -28,23 +28,44 @@ type Row = QueryRowsResponse['data'][number]
  * cast that would erase exactly the typing this keeps honest.
  */
 async function streamToFile(body: ReadableStream<Uint8Array>, file: WriteStream): Promise<void> {
-  const reader = body.getReader()
+  // Registered before the first write, not after the loop. `createWriteStream`
+  // opens lazily, so an EEXIST/EACCES/ENOSPC can surface at any point — with no
+  // listener attached it is an unhandled 'error' event that takes down the
+  // process instead of failing the download.
+  const failed = new Promise<never>((_resolve, reject) => {
+    file.once('error', reject)
+  })
+
+  const pump = (async () => {
+    const reader = body.getReader()
+    try {
+      while (true) {
+        const { done, value } = await reader.read()
+        if (done) break
+        // `write` returning false means the buffer is full; waiting for `drain`
+        // is what stops a large file being buffered entirely in memory.
+        if (!file.write(value)) await once(file, 'drain')
+      }
+    } finally {
+      reader.releaseLock()
+    }
+
+    await new Promise<void>((resolve) => file.end(resolve))
+  })()
+
   try {
-    while (true) {
-      const { done, value } = await reader.read()
-      if (done) break
-      // `write` returning false means the buffer is full; waiting for `drain` is
-      // what stops a large file being buffered entirely in memory.
-      if (!file.write(value)) await once(file, 'drain')
+    await Promise.race([pump, failed])
+  } catch (error) {
+    file.destroy()
+    const code = (error as NodeJS.ErrnoException).code
+    if (code === 'EEXIST') {
+      throw new SimApiError(
+        `${file.path} already exists. Pass --force to overwrite, or -o to write elsewhere.`,
+        0
+      )
     }
-  } finally {
-    reader.releaseLock()
+    throw new SimApiError(`Could not write ${file.path}: ${(error as Error).message}`, 0)
   }
-
-  await new Promise<void>((resolve, reject) => {
-    file.once('error', reject)
-    file.end(resolve)
-  })
 }
 
 /**
@@ -70,7 +91,8 @@ function rowColumns(rows: Row[]): Column<Row>[] {
       value: (row: Row) => {
         const value = row.data[key]
         if (value === null || value === undefined) return text(null)
-        return typeof value === 'object' ? JSON.stringify(value) : String(value)
+        // User-defined cell data is remote content; strip terminal controls.
+        return sanitize(typeof value === 'object' ? JSON.stringify(value) : String(value))
       },
     })),
   ]
@@ -89,36 +111,49 @@ export function attachHandWritten(program: Command): void {
     .command('download <fileId>')
     .description('Download a file')
     .option('-o, --output-file <path>', 'Where to write it (defaults to the file name)')
-    .action(async (fileId: string, options: { outputFile?: string }, command: Command) => {
-      const { client, profile } = clientFrom(command)
-      const workspaceId = client.requireWorkspace()
-
-      if (!profile.apiKey) {
-        throw new SimApiError(`Not logged in on profile "${profile.name}". Run: sim login`, 0)
-      }
-
-      const url = new URL(`${profile.endpoint}/api/v2/files/${encodeURIComponent(fileId)}`)
-      url.searchParams.set('workspaceId', workspaceId)
-
-      const response = await fetch(url, { headers: { 'x-api-key': profile.apiKey } })
-      if (!response.ok || !response.body) {
-        const raw = await response.text().catch(() => '')
-        throw new SimApiError(
-          raw || `Download failed with status ${response.status}`,
-          response.status
+    .option('--force', 'Overwrite the destination if it already exists')
+    .action(
+      async (
+        fileId: string,
+        options: { outputFile?: string; force?: boolean },
+        command: Command
+      ) => {
+        const { client, profile } = clientFrom(command)
+        const workspaceId = client.requireWorkspace()
+
+        if (!profile.apiKey) {
+          throw new SimApiError(`Not logged in on profile "${profile.name}". Run: sim login`, 0)
+        }
+
+        const url = new URL(`${profile.endpoint}/api/v2/files/${encodeURIComponent(fileId)}`)
+        url.searchParams.set('workspaceId', workspaceId)
+
+        const response = await fetch(url, { headers: { 'x-api-key': profile.apiKey } })
+        if (!response.ok || !response.body) {
+          const raw = await response.text().catch(() => '')
+          throw new SimApiError(
+            raw || `Download failed with status ${response.status}`,
+            response.status
+          )
+        }
+
+        const target =
+          options.outputFile ??
+          basename(
+            /filename="?([^";]+)"?/.exec(response.headers.get('content-disposition') ?? '')?.[1] ??
+              fileId
+          )
+
+        // `wx` fails rather than truncating: a download that silently replaces an
+        // existing file is unrecoverable, and the name often comes from the
+        // server's content-disposition rather than anything the caller typed.
+        await streamToFile(
+          response.body,
+          createWriteStream(target, { flags: options.force ? 'w' : 'wx' })
         )
+        console.log(chalk.green(`✓ Saved ${target}`))
       }
-
-      const target =
-        options.outputFile ??
-        basename(
-          /filename="?([^";]+)"?/.exec(response.headers.get('content-disposition') ?? '')?.[1] ??
-            fileId
-        )
-
-      await streamToFile(response.body, createWriteStream(target))
-      console.log(chalk.green(`✓ Saved ${target}`))
-    })
+    )
 
   // ── tables rows list ── columns come from user-defined row data ───────────
   const tables = group(program, 'tables')

packages/sim-cli/src/output/render.test.ts

@@ -7,10 +7,13 @@ import {
   duration,
   printList,
   printRecord,
+  sanitize,
   text,
   visibleWidth,
 } from './render.js'
 
+const ESC = String.fromCharCode(27)
+
 /** Colour is stripped when not writing to a TTY, so force it on for these assertions. */
 const coloured = new Chalk({ level: 1 })
 
@@ -189,3 +192,49 @@ describe('formatters', () => {
     expect(duration(90_000)).toBe('1m30s')
   })
 })
+
+describe('sanitize', () => {
+  // Remote content — knowledge document text, table cell values, workflow names —
+  // reaches an interactive terminal through the human-readable renderers.
+  it('removes an OSC window-title sequence', () => {
+    expect(sanitize(`${ESC}]0;pwned\u0007hello`)).toBe('hello')
+  })
+
+  it('removes OSC terminated by ST rather than BEL', () => {
+    expect(sanitize(`${ESC}]0;pwned${ESC}\\hello`)).toBe('hello')
+  })
+
+  it('removes cursor movement that would overwrite what was already printed', () => {
+    expect(sanitize(`before${ESC}[2A${ESC}[2Kafter`)).toBe('beforeafter')
+  })
+
+  it('removes a full terminal reset', () => {
+    expect(sanitize(`${ESC}creset`)).toBe('reset')
+  })
+
+  it('removes non-SGR CSI, which the old SGR-only pattern left executable', () => {
+    // The reported hole: stripping only `ESC [ … m` passed everything else through.
+    expect(sanitize(`${ESC}[6n`)).toBe('')
+    expect(sanitize(`${ESC}[?1049h`)).toBe('')
+  })
+
+  it('removes bare C0 and C1 control characters', () => {
+    expect(sanitize('a\u0000b\u0008c\u009bd')).toBe('abcd')
+  })
+
+  it('takes the following byte with a bare ESC, since ESC + printable is a sequence', () => {
+    expect(sanitize('a\u001bdb')).toBe('ab')
+  })
+
+  it('keeps tabs and newlines, which are legitimate content', () => {
+    expect(sanitize('a\tb\nc')).toBe('a\tb\nc')
+  })
+
+  it('leaves ordinary text untouched', () => {
+    expect(sanitize('refund policy — 30 days')).toBe('refund policy — 30 days')
+  })
+
+  it('is applied to values passing through text()', () => {
+    expect(text(`${ESC}]0;x\u0007safe`)).toBe('safe')
+  })
+})

packages/sim-cli/src/output/render.ts

@@ -13,9 +13,50 @@ const EMPTY_GLYPH = '—'
 /** Cell text for values that have no useful rendering, kept visually quiet. */
 const EMPTY = chalk.dim(EMPTY_GLYPH)
 
+/**
+ * Escape sequences and control characters that must never reach a terminal
+ * from server-supplied data.
+ *
+ * Covers CSI (`ESC [ … final`), OSC (`ESC ] … BEL|ST`), single-character escapes
+ * such as `ESC c` (full reset), and the bare C0/C1 control range. Anything a
+ * knowledge document, table cell, or workflow name contains is remote content —
+ * a document could set the window title, move the cursor to overwrite what was
+ * already printed, reset the terminal, or on some emulators drive clipboard and
+ * paste controls.
+ *
+ * Matching only SGR (`… m`) was the hole: it stripped colour and left every
+ * other sequence executable.
+ */
+const ESC = String.fromCharCode(27)
+const CONTROL_PATTERN = new RegExp(
+  [
+    `${ESC}\\][^\\u0007${ESC}]*(?:\\u0007|${ESC}\\\\)?`, // OSC … BEL or ST
+    `${ESC}\\[[0-9;?]*[ -/]*[@-~]`, // CSI … final byte
+    // Any other ESC + printable: `ESC c` (full reset), `ESC 7`/`ESC 8` (cursor
+    // save/restore), `ESC (0` (line-drawing charset), and the rest. ESC is never
+    // legitimate content, so the whole two-byte form goes. OSC and CSI are
+    // matched above, so they win at the same position.
+    `${ESC}[ -~]`,
+    `${ESC}`, // a lone ESC with nothing valid after it
+    '[\\u0000-\\u0008\\u000b\\u000c\\u000e-\\u001f\\u007f-\\u009f]', // C0/C1, keeping \t and \n
+  ].join('|'),
+  'g'
+)
+
+/**
+ * Removes terminal control sequences from a server-supplied string.
+ *
+ * Applied where API values become display text, so the colour the CLI adds
+ * afterwards still works — sanitizing the finished cell would strip our own
+ * formatting too.
+ */
+export function sanitize(value: string): string {
+  return value.replace(CONTROL_PATTERN, '')
+}
+
 export function text(value: unknown): string {
   if (value === null || value === undefined || value === '') return EMPTY
-  return String(value)
+  return sanitize(String(value))
 }
 
 /** ISO timestamps are the wire format everywhere; show them without the milliseconds. */