From 5b6e54fba8e3243ce9d66759a1fcfd98ff145ac6 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 16 Sep 2026 10:23:32 -0700 Subject: [PATCH] fix(cli): accept prompt defaults in onboarding wizard validators (#13520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The CLI onboarding wizard's custom-setup path collects server, database, storage, and secrets configuration through `@clack/prompts` text prompts, most of which show a sensible default > - `@clack/prompts` runs each prompt's `validate` callback on the raw typed value, and only substitutes `defaultValue` after validation passes — so pressing Enter to accept a shown default hands the validator an empty string > - Eleven of the wizard's validators reject the empty string, which makes their own displayed defaults unacceptable: accepting "Embedded PostgreSQL port: 54329" fails with "Port must be an integer between 1 and 65535", "Backup directory" fails with "Backup directory is required", and so on through every prompt in the custom path > - This pull request makes each affected validator accept empty input when a default exists, while keeping all real validation for typed input > - The benefit is that the custom-setup wizard is walkable by pressing Enter through the defaults, as the UI clearly intends ## Linked Issues or Issue Description No existing issue. Description follows the bug-report template: **What happened?** In `paperclipai onboard` custom setup, pressing Enter to accept a prompt's displayed default fails validation on eleven prompts. Confirmed live on the "Embedded PostgreSQL port" prompt (default 54329 → "Port must be an integer between 1 and 65535") and the "Backup directory" prompt (populated default path → "Backup directory is required"). The only way through is to retype every default by hand. **Expected behavior** Pressing Enter accepts the displayed default, as in every standard `@clack/prompts` flow. **Steps to reproduce** 1. `npx paperclipai onboard` → choose Custom setup. 2. At "Embedded PostgreSQL port", press Enter to accept the shown default. 3. Validation rejects it. Same for the backup directory, backup interval/retention, server port, bind host, storage directory, S3 bucket/region, and secrets key-file prompts. **Paperclip version or commit** Reproduced on `2026.915.0-canary.11`; the same validators exist in the latest stable (`v2026.831.1`) — long-standing, not a recent regression. **Deployment mode** Any (the bug is in the CLI wizard). **Installation method** `npx paperclipai onboard`. ## What Changed - Audited all 15 `validate:` callbacks across `cli/src/prompts/{database,server,storage,llm,secrets}.ts`. Eleven rejected the empty string while displaying a default; two were already fine (hostname CSV prompts, where empty parses to `[]`); one is an intentionally required password with no default (left alone); one (PostgreSQL connection string) and one (public base URL) are conditionally required — empty now passes only when a saved default exists, so fresh setups still enforce the field - Fix pattern: allow empty/undefined input at the top of each affected validator; every check for non-empty typed input is unchanged. One deliberate exception: the bind-host validator validates `(val || defaultHost)` so accepting the default still runs the loopback safety check rather than bypassing it - New `cli/src/__tests__/prompt-default-accept.test.ts` (repo-convention vitest + clack mock reproducing real submit semantics — validate raw `""`, then substitute the default): drives the four prompt modules end-to-end and unit-exercises each captured validator (empty accepted, garbage still rejected, required-when-fresh still rejected) ## Verification - `pnpm exec vitest run src/__tests__/prompt-default-accept.test.ts` in `cli/`: 13/13 pass; with the prompt fixes stashed: 12/13 fail — the suite reproduces the bug - Full cli suite: 483 passed; 15 failures are pre-existing/environmental on the clean tree too (macOS `/var` realpath mismatch in `worktree.test.ts`, one parallel-run flake that passes in isolation) - `pnpm run typecheck` in `cli/`: zero errors in `cli/src` (the 228 pre-existing errors in `../server/src` from missing prebuilt workspace dist are identical with and without the change) ## Risks - Low. Typed input validates exactly as before; whitespace-only input is still rejected (clack substitutes the default only for truly-empty input). The two conditionally-required prompts still hard-require a value on fresh setups - No migrations, no API surface changes; CLI-only ## Model Used Claude Fable 5 (`claude-fable-5`), extended thinking with tool use (Claude Code); implementation drafted by a subagent on the same model and reviewed before commit. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .../__tests__/prompt-default-accept.test.ts | 295 ++++++++++++++++++ cli/src/prompts/database.ts | 37 ++- cli/src/prompts/secrets.ts | 4 +- cli/src/prompts/server.ts | 18 +- cli/src/prompts/storage.ts | 17 +- 5 files changed, 344 insertions(+), 27 deletions(-) create mode 100644 cli/src/__tests__/prompt-default-accept.test.ts diff --git a/cli/src/__tests__/prompt-default-accept.test.ts b/cli/src/__tests__/prompt-default-accept.test.ts new file mode 100644 index 0000000000..1908a557c8 --- /dev/null +++ b/cli/src/__tests__/prompt-default-accept.test.ts @@ -0,0 +1,295 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as p from "@clack/prompts"; +import { promptDatabase } from "../prompts/database.js"; +import { promptSecrets } from "../prompts/secrets.js"; +import { promptServer } from "../prompts/server.js"; +import { promptStorage } from "../prompts/storage.js"; +import type { DatabaseConfig } from "../config/schema.js"; + +vi.mock("@clack/prompts", () => ({ + text: vi.fn(), + select: vi.fn(), + confirm: vi.fn(), + password: vi.fn(), + isCancel: vi.fn(() => false), + cancel: vi.fn(), + note: vi.fn(), + log: { + error: vi.fn(), + info: vi.fn(), + message: vi.fn(), + step: vi.fn(), + success: vi.fn(), + warn: vi.fn(), + }, +})); + +type CapturedTextOptions = { + message: string; + defaultValue?: string; + validate?: (value: string | undefined) => string | Error | undefined; +}; + +const capturedText: CapturedTextOptions[] = []; + +/** + * Emulates @clack/core submit semantics for pressing Enter without typing: + * the validator runs on the RAW input (the empty string) and defaultValue is + * substituted only after validation passes. A validator that rejects "" makes + * the shown default unacceptable. + */ +function pressEnterOnEveryTextPrompt() { + vi.mocked(p.text).mockImplementation(async (opts) => { + const options = opts as unknown as CapturedTextOptions; + capturedText.push(options); + const error = options.validate?.(""); + if (error) { + throw new Error(`"${options.message}" rejected pressing Enter on its default: ${String(error)}`); + } + return options.defaultValue ?? ""; + }); +} + +function queueSelects(values: unknown[]) { + const remaining = [...values]; + vi.mocked(p.select).mockImplementation(async () => { + if (remaining.length === 0) throw new Error("unexpected select prompt"); + return remaining.shift() as never; + }); +} + +function validatorFor(message: string): NonNullable { + const call = capturedText.find((options) => options.message === message); + if (!call?.validate) throw new Error(`no validator captured for "${message}"`); + return call.validate; +} + +const dbFixture: DatabaseConfig = { + mode: "postgres", + connectionString: "postgres://user:pass@localhost:5432/paperclip", + embeddedPostgresDataDir: "/var/lib/paperclip/db", + embeddedPostgresPort: 54329, + backup: { + enabled: false, + intervalMinutes: 30, + retentionDays: 7, + dir: "/var/lib/paperclip/backups", + }, +}; + +beforeEach(() => { + vi.clearAllMocks(); + capturedText.length = 0; + vi.mocked(p.isCancel).mockReturnValue(false); + vi.mocked(p.confirm).mockImplementation(async (opts) => (opts.initialValue ?? false) as never); + pressEnterOnEveryTextPrompt(); +}); + +describe("promptDatabase accepts defaults", () => { + it("accepts every shown default in the embedded-postgres flow", async () => { + queueSelects(["embedded-postgres"]); + + const db = await promptDatabase(); + + expect(db.mode).toBe("embedded-postgres"); + expect(db.embeddedPostgresPort).toBe(54329); + expect(db.embeddedPostgresDataDir).toBeTruthy(); + expect(db.backup.intervalMinutes).toBe(60); + expect(db.backup.retentionDays).toBe(30); + expect(db.backup.dir).toBeTruthy(); + }); + + it("keeps real validation for typed port, interval, and retention input", async () => { + queueSelects(["embedded-postgres"]); + await promptDatabase(); + + const port = validatorFor("Embedded PostgreSQL port"); + expect(port("")).toBeUndefined(); + expect(port(undefined)).toBeUndefined(); + expect(port("54329")).toBeUndefined(); + expect(port("0")).toBeTruthy(); + expect(port("70000")).toBeTruthy(); + expect(port("abc")).toBeTruthy(); + + const interval = validatorFor("Backup interval (minutes)"); + expect(interval("")).toBeUndefined(); + expect(interval("60")).toBeUndefined(); + expect(interval("0")).toBeTruthy(); + expect(interval("999999")).toBeTruthy(); + + const retention = validatorFor("Backup retention (days)"); + expect(retention("")).toBeUndefined(); + expect(retention("30")).toBeUndefined(); + expect(retention("0")).toBeTruthy(); + expect(retention("99999")).toBeTruthy(); + + const backupDir = validatorFor("Backup directory"); + expect(backupDir("")).toBeUndefined(); + expect(backupDir(" ")).toBeTruthy(); + }); + + it("accepts the saved connection string default when reconfiguring postgres mode", async () => { + queueSelects(["postgres"]); + + const db = await promptDatabase(dbFixture); + + expect(db.connectionString).toBe(dbFixture.connectionString); + }); + + it("still requires a connection string when no saved default exists", async () => { + queueSelects(["postgres"]); + + await expect(promptDatabase()).rejects.toThrow(/Connection string is required/); + + const connection = validatorFor("PostgreSQL connection string"); + expect(connection("postgres://user:pass@localhost:5432/paperclip")).toBeUndefined(); + expect(connection("mysql://nope")).toBeTruthy(); + }); +}); + +describe("promptServer accepts defaults", () => { + it("accepts the default port in the loopback flow", async () => { + queueSelects(["loopback"]); + + const { server } = await promptServer(); + + expect(server.port).toBe(3100); + + const port = validatorFor("Server port"); + expect(port("")).toBeUndefined(); + expect(port("8443")).toBeUndefined(); + expect(port("0")).toBeTruthy(); + expect(port("abc")).toBeTruthy(); + }); + + it("accepts empty optional hostnames in the lan flow", async () => { + queueSelects(["lan"]); + + const { server } = await promptServer(); + + expect(server.allowedHostnames).toEqual([]); + }); + + it("accepts the loopback default host in the custom local_trusted flow", async () => { + queueSelects(["custom", "local_trusted"]); + + const { server } = await promptServer(); + + expect(server.host).toBe("127.0.0.1"); + }); + + it("still rejects accepting a non-loopback saved host in local_trusted mode", async () => { + queueSelects(["custom", "local_trusted"]); + + await expect(promptServer({ currentServer: { host: "0.0.0.0" } })).rejects.toThrow(/loopback/); + }); + + it("accepts the saved public base URL default when reconfiguring a public deployment", async () => { + queueSelects(["custom", "authenticated", "public"]); + + const { auth } = await promptServer({ + currentServer: { host: "0.0.0.0", port: 8443 }, + currentAuth: { publicBaseUrl: "https://paperclip.example.com" }, + }); + + expect(auth.publicBaseUrl).toBe("https://paperclip.example.com"); + }); + + it("still requires a public base URL when no saved default exists", async () => { + queueSelects(["custom", "authenticated", "public"]); + + await expect(promptServer()).rejects.toThrow(/Public base URL is required/); + + const url = validatorFor("Public base URL"); + expect(url("https://paperclip.example.com")).toBeUndefined(); + expect(url("ftp://paperclip.example.com")).toBeTruthy(); + expect(url("not a url")).toBeTruthy(); + }); +}); + +describe("promptStorage accepts defaults", () => { + it("accepts the default base directory in the local_disk flow", async () => { + queueSelects(["local_disk"]); + + const storage = await promptStorage(); + + expect(storage.provider).toBe("local_disk"); + expect(storage.localDisk.baseDir).toBeTruthy(); + + const baseDir = validatorFor("Local storage base directory"); + expect(baseDir("")).toBeUndefined(); + expect(baseDir(" ")).toBeTruthy(); + }); + + it("accepts the default bucket and region in the s3 flow", async () => { + queueSelects(["s3"]); + + const storage = await promptStorage(); + + expect(storage.s3.bucket).toBe("paperclip"); + expect(storage.s3.region).toBe("us-east-1"); + + expect(validatorFor("S3 bucket")(" ")).toBeTruthy(); + expect(validatorFor("S3 region")(" ")).toBeTruthy(); + }); +}); + +describe("promptSecrets accepts defaults", () => { + it("accepts the default key file path in the local_encrypted flow", async () => { + queueSelects(["local_encrypted"]); + + const secrets = await promptSecrets(); + + expect(secrets.provider).toBe("local_encrypted"); + expect(secrets.localEncrypted.keyFilePath).toBeTruthy(); + + const keyPath = validatorFor("Local encrypted key file path"); + expect(keyPath("")).toBeUndefined(); + expect(keyPath(" ")).toBeTruthy(); + }); +}); + +describe("invalid saved or derived defaults are still validated", () => { + // Accepting a default must not bypass validation: the validators check the + // effective value (typed input, or the default), so a bad value from the + // environment or a hand-edited config errors at the prompt instead of + // blowing up later at config-schema parsing. + it("rejects accepting an out-of-range saved server port", async () => { + queueSelects(["loopback"]); + + await expect( + promptServer({ currentServer: { port: 70000 as never } }), + ).rejects.toThrow(/integer between 1 and 65535/); + }); + + it("rejects accepting a non-integer saved embedded PostgreSQL port", async () => { + queueSelects(["embedded-postgres"]); + + await expect( + promptDatabase({ ...dbFixture, mode: "embedded-postgres", embeddedPostgresPort: 12.5 as never }), + ).rejects.toThrow(/Port must be an integer/); + }); + + it("rejects accepting an invalid saved public base URL", async () => { + queueSelects(["custom", "authenticated", "public"]); + + await expect( + promptServer({ + currentServer: { host: "0.0.0.0", port: 8443 }, + currentAuth: { publicBaseUrl: "not a url" }, + }), + ).rejects.toThrow(/valid URL/); + }); + + it("rejects accepting a whitespace-only saved S3 bucket", async () => { + queueSelects(["s3"]); + + await expect( + promptStorage({ + provider: "s3", + localDisk: { baseDir: "" }, + s3: { bucket: " ", region: "us-east-1", endpoint: "", forcePathStyle: false }, + } as never), + ).rejects.toThrow(/Bucket is required/); + }); +}); diff --git a/cli/src/prompts/database.ts b/cli/src/prompts/database.ts index b4ba075f9a..21c604d7bf 100644 --- a/cli/src/prompts/database.ts +++ b/cli/src/prompts/database.ts @@ -39,15 +39,21 @@ export async function promptDatabase(current?: DatabaseConfig): Promise { - if (!val) return "Connection string is required for PostgreSQL mode"; - if (!val.startsWith("postgres")) return "Must be a postgres:// or postgresql:// URL"; + const candidate = val || connectionStringDefault; + if (!candidate) return "Connection string is required for PostgreSQL mode"; + if (!candidate.startsWith("postgres")) return "Must be a postgres:// or postgresql:// URL"; }, }); @@ -73,10 +79,10 @@ export async function promptDatabase(current?: DatabaseConfig): Promise { - const n = Number(val); + const n = Number(val || embeddedPortDefault); if (!Number.isInteger(n) || n < 1 || n > 65535) return "Port must be an integer between 1 and 65535"; }, }); @@ -86,7 +92,7 @@ export async function promptDatabase(current?: DatabaseConfig): Promise (!val || val.trim().length === 0 ? "Backup directory is required" : undefined), + validate: (val) => ((val || backupDirDefault).trim().length === 0 ? "Backup directory is required" : undefined), }); if (p.isCancel(backupDirInput)) { p.cancel("Setup cancelled."); process.exit(0); } + const backupIntervalDefault = String(base.backup.intervalMinutes || 60); + const backupRetentionDefault = String(base.backup.retentionDays || 30); const backupIntervalInput = await p.text({ message: "Backup interval (minutes)", - defaultValue: String(base.backup.intervalMinutes || 60), + defaultValue: backupIntervalDefault, placeholder: "60", validate: (val) => { - const n = Number(val); + const n = Number(val || backupIntervalDefault); if (!Number.isInteger(n) || n < 1) return "Interval must be a positive integer"; if (n > 10080) return "Interval must be 10080 minutes (7 days) or less"; return undefined; @@ -128,10 +137,10 @@ export async function promptDatabase(current?: DatabaseConfig): Promise { - const n = Number(val); + const n = Number(val || backupRetentionDefault); if (!Number.isInteger(n) || n < 1) return "Retention must be a positive integer"; if (n > 3650) return "Retention must be 3650 days or less"; return undefined; @@ -149,8 +158,8 @@ export async function promptDatabase(current?: DatabaseConfig): Promise { - if (!value || value.trim().length === 0) return "Key file path is required"; + // Clack validates the raw input before applying defaultValue — + // validate the value that will actually be submitted. + if ((value || keyFilePath).trim().length === 0) return "Key file path is required"; }, }); diff --git a/cli/src/prompts/server.ts b/cli/src/prompts/server.ts index bff9e18072..9844c8a040 100644 --- a/cli/src/prompts/server.ts +++ b/cli/src/prompts/server.ts @@ -50,12 +50,16 @@ export async function promptServer(opts?: { if (p.isCancel(bindSelection)) cancelled(); const bind = bindSelection as BindMode; + const portDefault = String(currentServer?.port ?? 3100); const portStr = await p.text({ message: "Server port", - defaultValue: String(currentServer?.port ?? 3100), + defaultValue: portDefault, placeholder: "3100", validate: (val) => { - const n = Number(val); + // Clack validates the raw input before applying defaultValue, so an + // Enter press hands the validator an empty string. Validate the value + // that will actually be submitted — the typed input, or the default. + const n = Number(val || portDefault); if (isNaN(n) || n < 1 || n > 65535 || !Number.isInteger(n)) { return "Must be an integer between 1 and 65535"; } @@ -156,8 +160,9 @@ export async function promptServer(opts?: { defaultValue: defaultHost, placeholder: defaultHost, validate: (val) => { - if (!val || !val.trim()) return "Host is required"; - if (deploymentMode === "local_trusted" && !isLoopbackHost(val.trim())) { + const candidate = (val || defaultHost).trim(); + if (!candidate) return "Host is required"; + if (deploymentMode === "local_trusted" && !isLoopbackHost(candidate)) { return "Local trusted mode requires a loopback host such as 127.0.0.1"; } }, @@ -187,12 +192,13 @@ export async function promptServer(opts?: { let publicBaseUrl: string | undefined; if (deploymentMode === "authenticated" && exposure === "public") { + const publicBaseUrlDefault = currentAuth?.publicBaseUrl ?? ""; const urlInput = await p.text({ message: "Public base URL", - defaultValue: currentAuth?.publicBaseUrl ?? "", + defaultValue: publicBaseUrlDefault, placeholder: "https://paperclip.example.com", validate: (val) => { - const candidate = val?.trim() ?? ""; + const candidate = (val || publicBaseUrlDefault).trim(); if (!candidate) return "Public base URL is required for public exposure"; try { const url = new URL(candidate); diff --git a/cli/src/prompts/storage.ts b/cli/src/prompts/storage.ts index be7556d0f1..7919261418 100644 --- a/cli/src/prompts/storage.ts +++ b/cli/src/prompts/storage.ts @@ -48,12 +48,15 @@ export async function promptStorage(current?: StorageConfig): Promise { - if (!value || value.trim().length === 0) return "Storage base directory is required"; + // Clack validates the raw input before applying defaultValue — + // validate the value that will actually be submitted. + if ((value || baseDirDefault).trim().length === 0) return "Storage base directory is required"; }, }); @@ -71,12 +74,14 @@ export async function promptStorage(current?: StorageConfig): Promise { - if (!value || value.trim().length === 0) return "Bucket is required"; + if ((value || bucketDefault).trim().length === 0) return "Bucket is required"; }, }); @@ -87,10 +92,10 @@ export async function promptStorage(current?: StorageConfig): Promise { - if (!value || value.trim().length === 0) return "Region is required"; + if ((value || regionDefault).trim().length === 0) return "Region is required"; }, });