From c664a5de763bfa83d88544a4703220e1b1a415b0 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 25 Aug 2026 01:28:23 -0700 Subject: [PATCH] fix(cli): deadline-based service wait, no new dependency, honest URL Address review and policy: drop the open dependency (platform opener via execFile - the lockfile is CI-owned and e2e installs frozen), count probe time against the readiness deadline, and announce the URL only when onboarding configured the default config path the supervisor actually reads - otherwise say which path the service reads and point at service status. --- cli/package.json | 1 - cli/src/__tests__/onboard-service.test.ts | 17 +++++++++++++-- cli/src/commands/onboard.ts | 26 +++++++++++++++++++---- cli/src/onboard-service.ts | 19 +++++++++++++---- pnpm-lock.yaml | 3 --- 5 files changed, 52 insertions(+), 14 deletions(-) diff --git a/cli/package.json b/cli/package.json index 28f394ca8f1..4113ed4d45e 100644 --- a/cli/package.json +++ b/cli/package.json @@ -56,7 +56,6 @@ "dotenv": "^17.4.2", "commander": "^13.1.0", "embedded-postgres": "^18.1.0-beta.16", - "open": "^11.0.1", "picocolors": "^1.1.1" }, "devDependencies": { diff --git a/cli/src/__tests__/onboard-service.test.ts b/cli/src/__tests__/onboard-service.test.ts index 7f6ab8b46e4..a24806fbdd5 100644 --- a/cli/src/__tests__/onboard-service.test.ts +++ b/cli/src/__tests__/onboard-service.test.ts @@ -165,10 +165,12 @@ describe("shouldOfferForegroundStart", () => { describe("announceServiceReady", () => { function readyDeps(overrides: Record = {}) { + let clock = 0; return { probeHealth: vi.fn(async () => true), openBrowser: vi.fn(async () => undefined), sleep: vi.fn(async () => undefined), + now: vi.fn(() => { clock += 10; return clock; }), info: vi.fn(), success: vi.fn(), warn: vi.fn(), @@ -188,11 +190,22 @@ describe("announceServiceReady", () => { it("polls until the service becomes healthy", async () => { const probeHealth = vi.fn(async () => false).mockResolvedValueOnce(false).mockResolvedValueOnce(false).mockResolvedValue(true); const deps = readyDeps({ probeHealth }); - const ready = await announceServiceReady({ baseUrl: "http://127.0.0.1:3100", interactive: true, timeoutMs: 50, pollIntervalMs: 10 }, deps); + const ready = await announceServiceReady({ baseUrl: "http://127.0.0.1:3100", interactive: true, timeoutMs: 100, pollIntervalMs: 10 }, deps); expect(ready).toBe(true); expect(probeHealth).toHaveBeenCalledTimes(3); }); + it("counts probe time against the deadline instead of only sleeps", async () => { + let clock = 0; + const deps = readyDeps({ + probeHealth: vi.fn(async () => { clock += 2_000; return false; }), + now: vi.fn(() => clock), + }); + const ready = await announceServiceReady({ baseUrl: "http://127.0.0.1:3100", interactive: true, timeoutMs: 6_000, pollIntervalMs: 1_000 }, deps); + expect(ready).toBe(false); + expect(deps.probeHealth).toHaveBeenCalledTimes(3); + }); + it("prints the URL instead of opening a browser on non-interactive runs", async () => { const deps = readyDeps(); await announceServiceReady({ baseUrl: "http://127.0.0.1:3100", interactive: false }, deps); @@ -209,7 +222,7 @@ describe("announceServiceReady", () => { it("warns with service diagnostics when the service never answers", async () => { const deps = readyDeps({ probeHealth: vi.fn(async () => false) }); - const ready = await announceServiceReady({ baseUrl: "http://127.0.0.1:3100", interactive: true, timeoutMs: 30, pollIntervalMs: 10 }, deps); + const ready = await announceServiceReady({ baseUrl: "http://127.0.0.1:3100", interactive: true, timeoutMs: 25, pollIntervalMs: 10 }, deps); expect(ready).toBe(false); expect(deps.openBrowser).not.toHaveBeenCalled(); const warned = (deps.warn as ReturnType).mock.calls[0]?.[0] as string; diff --git a/cli/src/commands/onboard.ts b/cli/src/commands/onboard.ts index 75d3597be93..cce0571c0d6 100644 --- a/cli/src/commands/onboard.ts +++ b/cli/src/commands/onboard.ts @@ -43,6 +43,7 @@ import { resolveDefaultBackupDir, resolveDefaultEmbeddedPostgresDir, resolveDefaultLogsDir, + resolveDefaultConfigPath, resolvePaperclipInstanceId, } from "../config/home.js"; import { bootstrapCeoInvite } from "./auth-bootstrap-ceo.js"; @@ -129,6 +130,23 @@ function resolvePathFromEnv(rawValue: string | undefined): string | null { return path.resolve(expandHomePrefix(rawValue.trim())); } +async function announceInstalledService(options: { configPath: string; baseUrl: string }): Promise { + const serviceConfigPath = resolveDefaultConfigPath(resolvePaperclipInstanceId()); + if (path.resolve(options.configPath) === path.resolve(serviceConfigPath)) { + await announceServiceReady({ + baseUrl: options.baseUrl, + interactive: Boolean(process.stdin.isTTY && process.stdout.isTTY), + }); + return; + } + p.log.info( + [ + `The background service reads ${serviceConfigPath}, not ${options.configPath}.`, + `Check ${pc.cyan("paperclipai service status")} for the served address.`, + ].join("\n"), + ); +} + function describeServerBinding(server: Pick): string { const bind = server.bind ?? inferBindModeFromHost(server.host); const detail = @@ -459,9 +477,9 @@ export async function onboard(opts: OnboardOptions): Promise { printManagedInstallHint(); const serviceInstalled = await handleOnboardService(opts); if (serviceInstalled) { - await announceServiceReady({ + await announceInstalledService({ + configPath, baseUrl: buildLocalBaseUrl(existingConfig.server.host, existingConfig.server.port), - interactive: Boolean(process.stdin.isTTY && process.stdout.isTTY), }); } @@ -731,9 +749,9 @@ export async function onboard(opts: OnboardOptions): Promise { const serviceInstalled = await handleOnboardService(opts); if (serviceInstalled) { - await announceServiceReady({ + await announceInstalledService({ + configPath, baseUrl: buildLocalBaseUrl(server.host, server.port), - interactive: Boolean(process.stdin.isTTY && process.stdout.isTTY), }); } diff --git a/cli/src/onboard-service.ts b/cli/src/onboard-service.ts index 645ae888211..95725783805 100644 --- a/cli/src/onboard-service.ts +++ b/cli/src/onboard-service.ts @@ -170,6 +170,7 @@ export type ServiceReadyDependencies = { probeHealth: (healthUrl: string) => Promise; openBrowser: (url: string) => Promise; sleep: (ms: number) => Promise; + now: () => number; info: (message: string) => void; success: (message: string) => void; warn: (message: string) => void; @@ -185,11 +186,18 @@ const defaultServiceReadyDependencies: ServiceReadyDependencies = { return false; } }, + // The platform opener keeps this dependency-free; a failure only means the + // caller prints the address instead. openBrowser: async (url) => { - const mod = await import("open"); - await mod.default(url); + const { execFile } = await import("node:child_process"); + const command = process.platform === "darwin" ? "open" : process.platform === "win32" ? "cmd" : "xdg-open"; + const args = process.platform === "win32" ? ["/c", "start", "", url] : [url]; + await new Promise((resolve, reject) => { + execFile(command, args, (error) => (error ? reject(error) : resolve())); + }); }, sleep: (ms) => new Promise((resolve) => setTimeout(resolve, ms)), + now: () => Date.now(), info: (message) => p.log.message(pc.dim(message)), success: (message) => p.log.success(message), warn: (message) => p.log.warn(message), @@ -208,8 +216,10 @@ export async function announceServiceReady( const pollIntervalMs = options.pollIntervalMs ?? 1_000; const healthUrl = `${options.baseUrl}/api/health`; deps.info(`Waiting for the server at ${options.baseUrl} ...`); - const attempts = Math.max(1, Math.ceil(timeoutMs / pollIntervalMs)); - for (let attempt = 0; attempt < attempts; attempt += 1) { + // Probe time counts against the deadline: a slow probe must not stretch + // the configured timeout. + const deadline = deps.now() + timeoutMs; + for (;;) { if (await deps.probeHealth(healthUrl)) { deps.success(`Paperclip is running at ${options.baseUrl}`); if (options.interactive) { @@ -223,6 +233,7 @@ export async function announceServiceReady( deps.info(`Open ${options.baseUrl} in your browser to get started.`); return true; } + if (deps.now() >= deadline) break; await deps.sleep(pollIntervalMs); } deps.warn( diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 617f23693af..88daa34971e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -109,9 +109,6 @@ importers: embedded-postgres: specifier: ^18.1.0-beta.16 version: 18.1.0-beta.16(patch_hash=55uhvnotpqyiy37rn3pqpukhei) - open: - specifier: ^11.0.1 - version: 11.0.1 picocolors: specifier: ^1.1.1 version: 1.1.1