fix(cli): reject a non-numeric or non-positive --chunk-size before encrypt backfill starts

.changeset/encrypt-backfill-chunk-size.md

@@ -0,0 +1,5 @@
+---
+'stash': patch
+---
+
+`stash encrypt backfill` now rejects a `--chunk-size` that is not a positive whole number (`abc`, `0`, `-5`, `1.5`) with a clear error and exit code 1, before loading the encryption client or opening a database connection. Previously the value reached the backfill engine unchecked: a non-numeric value became `NaN` and failed inside Postgres after the dual-write gate had already been recorded, under the generic "Backfill failed" message, and `0` reported a completed backfill having encrypted nothing. The default is unchanged at 1000 and is now shown in `--help`.

languages/typescript/packages/cli/src/bin/main.ts

@@ -19,6 +19,7 @@ import { readFileSync } from 'node:fs'
 import { dirname, join } from 'node:path'
 import { fileURLToPath } from 'node:url'
 import * as p from '@clack/prompts'
+import { InvalidChunkSizeError, parseChunkSize } from '../cli/chunk-size.js'
 import { CliExit } from '../cli/exit.js'
 import { renderCommandHelp } from '../cli/help.js'
 import { validateInstallFlags } from '../commands/db/install.js'
@@ -429,9 +430,7 @@ async function runEncryptCommand(
         table,
         column,
         pkColumn: values['pk-column'],
-        chunkSize: values['chunk-size']
-          ? Number(values['chunk-size'])
-          : undefined,
+        chunkSize: parseChunkSizeFlag(values['chunk-size']),
         encryptedColumn: values['encrypted-column'],
         schemaColumnKey: values['schema-column-key'],
         confirmDualWritesDeployed: flags['confirm-dual-writes-deployed'],
@@ -475,6 +474,24 @@ function requireValue(values: Record<string, string>, key: string): string {
   return v
 }
 
+/**
+ * Parse `--chunk-size` at the argv boundary, where the raw text is still in
+ * hand, so the error can quote exactly what was typed. The handler validates
+ * the number again through the same helper, for callers that bypass argv.
+ * Returns `undefined` when the flag is absent so the handler applies its
+ * own default.
+ */
+function parseChunkSizeFlag(raw: string | undefined): number | undefined {
+  if (raw === undefined) return undefined
+  try {
+    return parseChunkSize(raw)
+  } catch (error) {
+    if (!(error instanceof InvalidChunkSizeError)) throw error
+    p.log.error(error.message)
+    throw new CliExit(1)
+  }
+}
+
 async function runSchemaCommand(
   sub: string | undefined,
   flags: Record<string, boolean>,

languages/typescript/packages/cli/src/cli/__tests__/chunk-size.test.ts

@@ -0,0 +1,86 @@
+/**
+ * `--chunk-size` is the one backfill flag whose bad values do not fail at the
+ * prompt: `Number('abc')` is `NaN`, and the engine binds whatever it is given
+ * straight into `LIMIT $n`. These tests pin the single decision — what counts
+ * as a chunk size — that both `encrypt backfill` and `decrypt backfill` defer
+ * to, in both the forms it is reached with: raw argv text and a coerced
+ * number.
+ */
+
+import { describe, expect, it } from 'vitest'
+import {
+  InvalidChunkSizeError,
+  parseChunkSize,
+  resolveChunkSize,
+} from '../chunk-size.js'
+
+describe('parseChunkSize', () => {
+  it.each([
+    ['500', 500],
+    ['1', 1],
+    [' 42 ', 42],
+    ['007', 7],
+    [500, 500],
+    [1, 1],
+  ])('accepts %j as %d', (input, expected) => {
+    expect(parseChunkSize(input)).toBe(expected)
+  })
+
+  it.each([
+    'abc',
+    '',
+    '   ',
+    '0',
+    '-5',
+    '1.5',
+    '1e3',
+    '0x10',
+    'Infinity',
+    'NaN',
+    '12abc',
+  ])('rejects the text %j', (input) => {
+    expect(() => parseChunkSize(input)).toThrow(InvalidChunkSizeError)
+  })
+
+  it.each([
+    Number('abc'),
+    0,
+    -5,
+    1.5,
+    Number.POSITIVE_INFINITY,
+    Number.MAX_SAFE_INTEGER + 1,
+  ])('rejects the number %s', (input) => {
+    expect(() => parseChunkSize(input)).toThrow(InvalidChunkSizeError)
+  })
+
+  it('names the flag and quotes exactly what was typed', () => {
+    expect(() => parseChunkSize('abc')).toThrow(
+      '--chunk-size must be a positive whole number of rows (got "abc").',
+    )
+  })
+
+  it('reports a coerced number as the number it became', () => {
+    // What a handler sees after argv's `Number(...)` — the raw text is gone,
+    // so the message says what arrived rather than inventing a spelling.
+    expect(() => parseChunkSize(Number('abc'))).toThrow('(got NaN)')
+    expect(() => parseChunkSize(0)).toThrow('(got 0)')
+    expect(() => parseChunkSize(1.5)).toThrow('(got 1.5)')
+  })
+})
+
+describe('resolveChunkSize', () => {
+  it('applies the fallback only when the flag was not given', () => {
+    expect(resolveChunkSize(undefined, 1000)).toBe(1000)
+    expect(resolveChunkSize(250, 1000)).toBe(250)
+    expect(resolveChunkSize('250', 1000)).toBe(250)
+  })
+
+  it('does not let a bad value fall back to the default', () => {
+    // A typo must be an error, not a silent 1000-row run.
+    expect(() => resolveChunkSize(Number('abc'), 1000)).toThrow(
+      InvalidChunkSizeError,
+    )
+    expect(() => resolveChunkSize('', 1000)).toThrow(InvalidChunkSizeError)
+    expect(() => resolveChunkSize(0, 1000)).toThrow(InvalidChunkSizeError)
+  })
+})

languages/typescript/packages/cli/src/cli/chunk-size.ts

@@ -0,0 +1,69 @@
+/**
+ * `--chunk-size` validation, shared by every backfill-shaped command.
+ *
+ * The flag arrives as text from argv and is coerced with `Number(...)`, so
+ * `--chunk-size abc` used to reach the backfill engine as `NaN`. The engine
+ * binds it straight into the page query's `LIMIT $n`, so Postgres refused it
+ * — but only after the dual-write gate had been recorded and the manifest
+ * written, and under the generic "Backfill failed" message that deliberately
+ * hides the database's text. Negatives and fractions failed the same way.
+ * `0` was worse: `LIMIT 0` returns no rows, which the engine reads as
+ * "nothing left to do" and reports as a completed backfill.
+ *
+ * This module is the one place that decides what a chunk size is, so
+ * `encrypt backfill` and `decrypt backfill` cannot disagree.
+ */
+
+/**
+ * Thrown by {@link parseChunkSize} for a value that is not a positive whole
+ * number. The message is author-controlled, names the flag and quotes exactly
+ * what was received, and carries no row data — safe to print verbatim.
+ */
+export class InvalidChunkSizeError extends Error {
+  constructor(message: string) {
+    super(message)
+    this.name = 'InvalidChunkSizeError'
+  }
+}
+
+/**
+ * Parse a `--chunk-size` value into the number of rows per chunk.
+ *
+ * Accepts the flag's raw argv text (so the error can quote what the user
+ * typed) or an already-coerced number (what a command handler receives).
+ * Anything that is not a positive whole number — `abc`, `0`, `-5`, `1.5`,
+ * `NaN`, `1e3`, an empty string — throws {@link InvalidChunkSizeError}.
+ *
+ * Text is matched as plain decimal digits rather than through `Number(...)`,
+ * so `0x10`, `1e3` and `Infinity` are rejected: a chunk size is a row count,
+ * and the only sensible spelling is one a user would read back the same way.
+ */
+export function parseChunkSize(input: string | number): number {
+  const value =
+    typeof input === 'string'
+      ? /^\s*\d+\s*$/.test(input)
+        ? Number(input)
+        : Number.NaN
+      : input
+
+  if (!Number.isSafeInteger(value) || value < 1) {
+    const received =
+      typeof input === 'string' ? JSON.stringify(input) : String(input)
+    throw new InvalidChunkSizeError(
+      `--chunk-size must be a positive whole number of rows (got ${received}).`,
+    )
+  }
+
+  return value
+}
+
+/**
+ * {@link parseChunkSize} with a default: `undefined` (the flag was not
+ * given) yields `fallback`, anything else is validated.
+ */
+export function resolveChunkSize(
+  input: string | number | undefined,
+  fallback: number,
+): number {
+  return input === undefined ? fallback : parseChunkSize(input)
+}

languages/typescript/packages/cli/src/cli/registry.ts

@@ -596,7 +596,8 @@ export const registry: CommandGroup[] = [
           {
             name: '--chunk-size',
             value: '<n>',
-            description: 'Rows encrypted per batch.',
+            description: 'Rows encrypted per batch; a positive whole number.',
+            default: '1000',
           },
           {
             name: '--encrypted-column',

languages/typescript/packages/cli/src/commands/encrypt/__tests__/backfill-chunk-size.test.ts

@@ -0,0 +1,169 @@
+/**
+ * Pins that `stash encrypt backfill` refuses a bad `--chunk-size` BEFORE it
+ * does anything else.
+ *
+ * The order matters as much as the refusal. The handler loads the stash
+ * config, loads the user's encryption client (jiti, credentials), opens a
+ * pg pool, and runs the dual-write gate — which RECORDS its acceptance —
+ * before the engine ever sees the chunk size. A `NaN` that was only caught
+ * at `LIMIT $n` had already paid for all of that and left a `dual_writing`
+ * event behind. So each test asserts the exit AND that none of those steps
+ * ran.
+ */
+
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const poolCtor = vi.hoisted(() => vi.fn())
+vi.mock('pg', () => ({
+  default: {
+    Pool: class {
+      constructor(config: unknown) {
+        poolCtor(config)
+      }
+      connect = vi.fn(async () => {
+        throw new Error('the pool must never be reached')
+      })
+      end = vi.fn(async () => {})
+    },
+  },
+}))
+
+// Only `installMigrationsSchema` and `runBackfill` need to be observable;
+// everything else stays real so a renamed export fails loudly.
+const migrateMocks = vi.hoisted(() => ({
+  installMigrationsSchema: vi.fn(async () => {}),
+  runBackfill: vi.fn(async () => {
+    throw new Error('the engine must never be reached')
+  }),
+}))
+vi.mock('@cipherstash/migrate', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@cipherstash/migrate')>()),
+  ...migrateMocks,
+}))
+
+vi.mock('@clack/prompts', () => ({
+  intro: vi.fn(),
+  outro: vi.fn(),
+  note: vi.fn(),
+  confirm: vi.fn(async () => true),
+  isCancel: vi.fn(() => false),
+  log: {
+    info: vi.fn(),
+    success: vi.fn(),
+    warn: vi.fn(),
+    error: vi.fn(),
+    step: vi.fn(),
+  },
+}))
+
+const loadStashConfig = vi.hoisted(() =>
+  vi.fn(async () => ({ databaseUrl: 'postgres://test' })),
+)
+vi.mock('@/config/index.js', () => ({ loadStashConfig }))
+
+const loadEncryptionContext = vi.hoisted(() =>
+  vi.fn(async () => ({ client: {}, tables: new Map() })),
+)
+vi.mock('../context.js', () => ({
+  loadEncryptionContext,
+  requireTable: vi.fn(),
+}))
+
+vi.mock('@/commands/init/utils.js', () => ({
+  detectPackageManager: vi.fn(() => 'npm'),
+  runnerCommand: vi.fn((_pm: string, ref: string) => `npx ${ref}`),
+}))
+
+import * as p from '@clack/prompts'
+import { backfillCommand } from '../backfill.js'
+
+/**
+ * `process.exit` must THROW here, not return: the real one never returns,
+ * and the handler's control flow relies on that. A no-op stub would let the
+ * run continue past the guard and fail the "nothing else ran" assertions for
+ * the wrong reason.
+ */
+function spyExit() {
+  return vi.spyOn(process, 'exit').mockImplementation(((code?: number) => {
+    throw new Error(`exit ${code}`)
+  }) as never)
+}
+
+describe('encrypt backfill — --chunk-size validation', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it.each([
+    ['the NaN that `--chunk-size abc` coerces to', Number('abc')],
+    ['zero', 0],
+    ['a negative', -5],
+    ['a fraction', 1.5],
+  ])(
+    'rejects %s with exit code 1 before touching anything',
+    async (_, chunkSize) => {
+      const exitSpy = spyExit()
+
+      await expect(
+        backfillCommand({ table: 'users', column: 'email', chunkSize }),
+      ).rejects.toThrow('exit 1')
+
+      expect(p.log.error).toHaveBeenCalledWith(
+        expect.stringContaining('--chunk-size must be a positive whole number'),
+      )
+      // Nothing paid for, nothing recorded.
+      expect(loadStashConfig).not.toHaveBeenCalled()
+      expect(loadEncryptionContext).not.toHaveBeenCalled()
+      expect(poolCtor).not.toHaveBeenCalled()
+      expect(migrateMocks.installMigrationsSchema).not.toHaveBeenCalled()
+      expect(migrateMocks.runBackfill).not.toHaveBeenCalled()
+
+      exitSpy.mockRestore()
+    },
+  )
+
+  it('quotes the offending value so the typo is recognisable', async () => {
+    const exitSpy = spyExit()
+
+    await expect(
+      backfillCommand({ table: 'users', column: 'email', chunkSize: 0 }),
+    ).rejects.toThrow('exit 1')
+
+    expect(p.log.error).toHaveBeenCalledWith(expect.stringContaining('(got 0)'))
+    exitSpy.mockRestore()
+  })
+
+  it('lets a valid chunk size through to the rest of the command', async () => {
+    const exitSpy = spyExit()
+
+    // Past the guard, the first thing the handler does is load the config;
+    // reaching it proves the guard accepted the value. The pool's `connect`
+    // then throws, which the handler reports and exits on — that path is not
+    // under test here, only that the guard did not fire.
+    await expect(
+      backfillCommand({ table: 'users', column: 'email', chunkSize: 250 }),
+    ).rejects.toThrow('exit 1')
+
+    expect(p.log.error).not.toHaveBeenCalledWith(
+      expect.stringContaining('--chunk-size'),
+    )
+    expect(loadStashConfig).toHaveBeenCalledTimes(1)
+    expect(loadEncryptionContext).toHaveBeenCalledTimes(1)
+    expect(poolCtor).toHaveBeenCalledTimes(1)
+    exitSpy.mockRestore()
+  })
+
+  it('applies the default when the flag is absent', async () => {
+    const exitSpy = spyExit()
+
+    await expect(
+      backfillCommand({ table: 'users', column: 'email' }),
+    ).rejects.toThrow('exit 1')
+
+    expect(p.log.error).not.toHaveBeenCalledWith(
+      expect.stringContaining('--chunk-size'),
+    )
+    expect(loadStashConfig).toHaveBeenCalledTimes(1)
+    exitSpy.mockRestore()
+  })
+})