From 38435b12610c3d453c941f37c6af7b1e39ff84e8 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Thu, 13 Aug 2026 14:34:31 -0700 Subject: [PATCH 1/5] feat(services): answer `services add` with no type by asking what to add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `insta services add` with no arguments said "error: missing required argument 'type'", and the three kinds were discoverable only by guessing one wrong and reading assertType's "type must be postgres|storage|compute". A project's menu should not be a punishment for guessing. Missing arguments now resolve before the command runs (src/resolve-service.ts): a terminal gets the two questions the dashboard's Add Service asks — what to add, then what to call it, prefilled per kind and validated with the command's own assertServiceName — and anything without a TTY gets the kind list as an error, non-zero, because nothing was created and exit 0 would read as success. No flags are prompted for: postgres and storage need none, and an empty compute is a legitimate service until `insta deploy`. Prompts are @clack/prompts, matching the InsForge CLI's `create` flow, gated to a real TTY so an agent can never block on one. An unknown type still passes straight through to assertType, so bad-type wording stays in one place. --- package-lock.json | 29 +++++++++++- package.json | 1 + src/index.ts | 10 +++- src/resolve-service.ts | 89 ++++++++++++++++++++++++++++++++++++ test/resolve-service.test.ts | 67 +++++++++++++++++++++++++++ 5 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 src/resolve-service.ts create mode 100644 test/resolve-service.test.ts diff --git a/package-lock.json b/package-lock.json index be6996b..4b99863 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,6 +9,7 @@ "version": "0.0.31", "license": "Apache-2.0", "dependencies": { + "@clack/prompts": "^0.9.1", "commander": "^12.1.0" }, "bin": { @@ -24,6 +25,27 @@ "node": ">=18" } }, + "node_modules/@clack/core": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/@clack/core/-/core-0.4.1.tgz", + "integrity": "sha512-Pxhij4UXg8KSr7rPek6Zowm+5M22rbd2g1nfojHJkxp5YkFqiZ2+YLEM/XGVIzvGOcM0nqjIFxrpDwWRZYWYjA==", + "license": "MIT", + "dependencies": { + "picocolors": "^1.0.0", + "sisteransi": "^1.0.5" + } + }, + "node_modules/@clack/prompts": { + "version": "0.9.1", + "resolved": "https://registry.npmjs.org/@clack/prompts/-/prompts-0.9.1.tgz", + "integrity": "sha512-JIpyaboYZeWYlyP0H+OoPPxd6nqueG/CmN6ixBiNFsIDHREevjIf0n0Ohh5gr5C8pEDknzgvz+pIJ8dMhzWIeg==", + "license": "MIT", + "dependencies": { + "@clack/core": "0.4.1", + "picocolors": "^1.0.0", + "sisteransi": "^1.0.5" + } + }, "node_modules/@esbuild/aix-ppc64": { "version": "0.28.1", "resolved": "https://registry.npmjs.org/@esbuild/aix-ppc64/-/aix-ppc64-0.28.1.tgz", @@ -1185,7 +1207,6 @@ "version": "1.1.1", "resolved": "https://registry.npmjs.org/picocolors/-/picocolors-1.1.1.tgz", "integrity": "sha512-xceH2snhtb5M9liqDsmEw56le376mTZkEX/jEb/RxNFyegNul7eNslCXP9FDj/Lcu0X8KEyMceP2ntpaHrDEVA==", - "dev": true, "license": "ISC" }, "node_modules/postcss": { @@ -1269,6 +1290,12 @@ "dev": true, "license": "ISC" }, + "node_modules/sisteransi": { + "version": "1.0.5", + "resolved": "https://registry.npmjs.org/sisteransi/-/sisteransi-1.0.5.tgz", + "integrity": "sha512-bLGGlR1QxBcynn2d5YmDX4MGjlZvy2MRBDRNHLJ8VI6l6+9FUiyTFNJ0IveOSP0bcXgVDPRcfGqA0pjaqUpfVg==", + "license": "MIT" + }, "node_modules/source-map-js": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/source-map-js/-/source-map-js-1.2.1.tgz", diff --git a/package.json b/package.json index 1e220b7..115a681 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ "prepublishOnly": "npm run build" }, "dependencies": { + "@clack/prompts": "^0.9.1", "commander": "^12.1.0" }, "devDependencies": { diff --git a/src/index.ts b/src/index.ts index 35c8e49..b2bcfc7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,6 +13,7 @@ import * as org from './commands/org.js' import * as project from './commands/project.js' import * as branch from './commands/branch.js' import * as services from './commands/services.js' +import { resolveServiceArgs, serviceArgsDeps } from './resolve-service.js' import * as regions from './commands/regions.js' import * as secretsCmd from './commands/secrets.js' import { deploy } from './commands/deploy.js' @@ -116,7 +117,9 @@ br.command('merge ').description('Merge a branch service set into anothe // ---- services (opt-in postgres/storage/compute) ---- const svc = program.command('services').alias('svc').description('Manage project services (postgres|storage|compute)') -svc.command('add ').description('Provision a service on demand (assigns a default domain for postgres/compute)') +// [type] [name] are optional so the command can answer "what can I add?" — a terminal is walked +// through the two questions, anything else gets the kind list back as an error (resolve-service.ts). +svc.command('add [type] [name]').description('Provision a service on demand (assigns a default domain for postgres/compute); with no type/name, a terminal picks from the service kinds') .option('--branch ', 'target branch (default: current)') .option('--region ', 'region for postgres/compute, e.g. us-east (see `insta regions`)') .option('--public', 'storage only: serve the bucket with anonymous public-read (default private)') @@ -124,7 +127,10 @@ svc.command('add ').description('Provision a service on demand (ass .option('--port ', 'compute only: port the image listens on (default 8080)') .option('--always-on', 'compute only: create as always-on — never scales to zero (all plans; billing is actual usage either way)') .option('--volume ', 'compute only: attach a persistent /data volume of this many whole Gi (also attachable later: `insta compute volume --size `; any plan may attach at the default 1; larger sizes are paid and plan-capped). Volume services keep 1 machine and stop (cold wake) instead of suspend when idle') - .action(guard((type, name, o) => services.servicesAdd(type, name, o))) + .action(guard(async (type, name, o) => { + const a = await resolveServiceArgs(type, name, serviceArgsDeps()) + return services.servicesAdd(a.type, a.name, o) + })) svc.command('list').option('--json').option('--branch ', 'branch (default: current)') .action(guard((o) => services.servicesList(o))) svc.command('remove ').description('Remove a service and destroy its resources') diff --git a/src/resolve-service.ts b/src/resolve-service.ts new file mode 100644 index 0000000..90bdda2 --- /dev/null +++ b/src/resolve-service.ts @@ -0,0 +1,89 @@ +// `insta services add` with no type (or no name): the three service kinds are otherwise only +// discoverable by guessing wrong and reading `type must be postgres|storage|compute`, so missing +// arguments answer "what can I add?" instead. A terminal gets the two questions the dashboard's +// Add Service asks — what, then what to call it; an agent gets the same list as an error, because +// nothing was created and a silent exit 0 would read as success. No flags are asked for: postgres +// and storage need none, and a compute service is legitimately empty until `insta deploy`. +import * as clack from '@clack/prompts' +import { SERVICE_TYPES, assertServiceName, type ServiceType } from './commands/services.js' + +export type ServiceKind = { type: ServiceType; hint: string; defaultName: string } + +export const SERVICE_KINDS: readonly ServiceKind[] = [ + { type: 'postgres', hint: 'relational DB, usable as soon as it is added', defaultName: 'main-db' }, + { type: 'storage', hint: 'S3-compatible bucket, private by default', defaultName: 'assets' }, + { type: 'compute', hint: 'an app to deploy code to (empty until `insta deploy`)', defaultName: 'app' }, +] + +export type ServiceArgsDeps = { + selectType: (kinds: readonly ServiceKind[]) => Promise + askName: (kind: ServiceKind) => Promise + tty: boolean +} + +/** The kind list, one line each — what a terminal picks from and an agent reads. */ +export function serviceKindLines(): string[] { + return SERVICE_KINDS.map((k) => ` ${k.type.padEnd(9)} ${k.hint}`) +} + +/** What to say when there is no terminal to ask: the missing half, and how to supply it. */ +export function missingArgsMessage(type?: string): string { + const known = SERVICE_KINDS.find((k) => k.type === type) + if (known) return `name the service: insta services add ${known.type} ${known.defaultName}` + return ['what to add:', ...serviceKindLines(), '', ' e.g. insta services add postgres main-db'].join('\n') +} + +/** + * Fill in whatever `insta services add` was not given. An unknown type passes straight through so + * `assertType` — not this — reports it, keeping one wording for a bad type everywhere. + */ +export async function resolveServiceArgs( + type: string | undefined, + name: string | undefined, + deps: ServiceArgsDeps, +): Promise<{ type: string; name: string }> { + if (type && name) return { type, name } + if (type && !SERVICE_TYPES.includes(type as ServiceType)) return { type, name: name ?? '' } + if (!deps.tty) throw new Error(missingArgsMessage(type)) + const picked = type ? (type as ServiceType) : await deps.selectType(SERVICE_KINDS) + const kind = SERVICE_KINDS.find((k) => k.type === picked) + if (!kind) return { type: picked, name: name ?? '' } + return { type: picked, name: name ?? (await deps.askName(kind)) } +} + +/** Real prompts (clack, as the InsForge CLI's `create`); cancelling exits without provisioning. */ +export async function promptServiceType(kinds: readonly ServiceKind[]): Promise { + const picked = await clack.select({ + message: 'What do you want to add?', + options: kinds.map((k) => ({ value: k.type, label: k.type, hint: k.hint })), + }) + if (clack.isCancel(picked)) process.exit(0) + return picked +} + +export async function promptServiceName(kind: ServiceKind): Promise { + const answer = await clack.text({ + message: `Name this ${kind.type} service:`, + initialValue: kind.defaultName, + // The same rule the command enforces, reported before Enter rather than after a round trip. + validate: (v) => { + try { + assertServiceName(v.trim()) + return undefined + } catch (e) { + return (e as Error).message + } + }, + }) + if (clack.isCancel(answer)) process.exit(0) + return answer.trim() +} + +/** Prompts on a real terminal only — an agent's stdin is not one, and must never block. */ +export function serviceArgsDeps(): ServiceArgsDeps { + return { + selectType: promptServiceType, + askName: promptServiceName, + tty: !!process.stdin.isTTY && !!process.stdout.isTTY, + } +} diff --git a/test/resolve-service.test.ts b/test/resolve-service.test.ts new file mode 100644 index 0000000..f7cc02f --- /dev/null +++ b/test/resolve-service.test.ts @@ -0,0 +1,67 @@ +// `insta services add` used to answer a missing type with commander's "missing required argument", +// which never says what the types are. Resolution: both args given → untouched (no prompt anywhere +// near the fast path); TTY → ask what, then what to call it; no TTY → the kind list as an error, +// because nothing was created; a bad type → straight through, so assertType keeps owning that +// wording. +import { test, expect } from 'vitest' +import { + SERVICE_KINDS, + missingArgsMessage, + resolveServiceArgs, + serviceKindLines, + type ServiceArgsDeps, +} from '../src/resolve-service.js' +import { SERVICE_TYPES } from '../src/commands/services.js' + +const deps = (over: Partial = {}): ServiceArgsDeps => ({ + selectType: async () => { throw new Error('selectType must not be called') }, + askName: async () => { throw new Error('askName must not be called') }, + tty: true, + ...over, +}) + +test('every service type has a kind entry to offer', () => { + expect(SERVICE_KINDS.map((k) => k.type)).toEqual([...SERVICE_TYPES]) +}) + +test('both arguments given: returned as-is, nothing is asked', async () => { + const r = await resolveServiceArgs('postgres', 'main-db', deps()) + expect(r).toEqual({ type: 'postgres', name: 'main-db' }) +}) + +test('no arguments + TTY: asks what, then the name for that kind', async () => { + const asked: string[] = [] + const r = await resolveServiceArgs(undefined, undefined, deps({ + selectType: async (kinds) => { asked.push('type'); return kinds[1]!.type }, + askName: async (kind) => { asked.push(`name:${kind.type}`); return kind.defaultName }, + })) + expect(r).toEqual({ type: 'storage', name: 'assets' }) + expect(asked).toEqual(['type', 'name:storage']) +}) + +test('type given, name missing + TTY: only the name is asked', async () => { + const r = await resolveServiceArgs('compute', undefined, deps({ askName: async (k) => k.defaultName })) + expect(r).toEqual({ type: 'compute', name: 'app' }) +}) + +test('no TTY: throws, and the message lists every kind with an example', async () => { + await expect(resolveServiceArgs(undefined, undefined, deps({ tty: false }))).rejects.toThrow(/what to add/) + const msg = missingArgsMessage() + for (const t of SERVICE_TYPES) expect(msg).toContain(t) + expect(msg).toContain('insta services add postgres main-db') +}) + +test('no TTY with a type: asks for the missing half, not the whole list', () => { + expect(missingArgsMessage('storage')).toBe('name the service: insta services add storage assets') +}) + +test('unknown type: passed through for assertType to report, prompts untouched', async () => { + const r = await resolveServiceArgs('mysql', undefined, deps({ tty: false })) + expect(r).toEqual({ type: 'mysql', name: '' }) +}) + +test('kind lines stay one per type and mention what each is', () => { + const lines = serviceKindLines() + expect(lines).toHaveLength(SERVICE_TYPES.length) + expect(lines.join('\n')).toContain('empty until `insta deploy`') +}) From 6535ccadc1897a17c80450344c4812e8343e76cd Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Thu, 13 Aug 2026 17:17:13 -0700 Subject: [PATCH 2/5] =?UTF-8?q?feat(services):=20`services=20add=20--json`?= =?UTF-8?q?=20=E2=80=94=20the=20created=20service,=20machine-readable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `add` was the only `services` command that produces an object it could not print: list / rename / set-access / scale / upgrade all take --json, so an agent that had just created a service still had to re-query `services list --json` to learn the id and domain it was given. --json also opts out of the prompts the parent branch added: a caller that asked for parseable stdout gets the same kind-list error a non-terminal gets, rather than a question that would corrupt the output and hang an agent that happens to own a TTY. `remove` stays without --json on purpose — it destroys rather than produces, and matches `branch delete` / `project delete`, which take none either. --- src/commands/services.ts | 3 ++- src/index.ts | 3 ++- src/resolve-service.ts | 5 +++-- test/resolve-service.test.ts | 14 ++++++++++++++ 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/commands/services.ts b/src/commands/services.ts index e5bdf03..9717b7f 100644 --- a/src/commands/services.ts +++ b/src/commands/services.ts @@ -62,7 +62,7 @@ export function resolveComputeServiceId(services: Array<{ id: string; type: stri // ---- commands ---- -export type ServicesAddOpts = { branch?: string; public?: boolean; image?: string; port?: string; region?: string; alwaysOn?: boolean; volume?: string } +export type ServicesAddOpts = { branch?: string; public?: boolean; image?: string; port?: string; region?: string; alwaysOn?: boolean; volume?: string; json?: boolean } // Map service-add options to the platform POST body. Pure, so it's unit-tested without a network // mock (mirrors deployRequestBody in deploy.ts). Validation (which options are valid for which @@ -93,6 +93,7 @@ export async function servicesAdd(type: string, name: string, opts: ServicesAddO const branch = opts.branch ?? p.branch const res = await api.rawRequest('POST', `/projects/${p.projectId}/services`, servicesAddRequestBody(type, name, branch, opts)) if (handleApproval(res)) return + if (opts.json) return printJson(res.body.service) const svc = res.body.service const access = svc.type === 'storage' ? ` [${svc.public ? 'public' : 'private'}]` : '' const img = svc.image ? ` running ${svc.image}${svc.port ? `:${svc.port}` : ''}` : '' diff --git a/src/index.ts b/src/index.ts index b2bcfc7..54f1201 100644 --- a/src/index.ts +++ b/src/index.ts @@ -127,8 +127,9 @@ svc.command('add [type] [name]').description('Provision a service on demand (ass .option('--port ', 'compute only: port the image listens on (default 8080)') .option('--always-on', 'compute only: create as always-on — never scales to zero (all plans; billing is actual usage either way)') .option('--volume ', 'compute only: attach a persistent /data volume of this many whole Gi (also attachable later: `insta compute volume --size `; any plan may attach at the default 1; larger sizes are paid and plan-capped). Volume services keep 1 machine and stop (cold wake) instead of suspend when idle') + .option('--json') .action(guard(async (type, name, o) => { - const a = await resolveServiceArgs(type, name, serviceArgsDeps()) + const a = await resolveServiceArgs(type, name, serviceArgsDeps(o.json)) return services.servicesAdd(a.type, a.name, o) })) svc.command('list').option('--json').option('--branch ', 'branch (default: current)') diff --git a/src/resolve-service.ts b/src/resolve-service.ts index 90bdda2..61f2ffc 100644 --- a/src/resolve-service.ts +++ b/src/resolve-service.ts @@ -80,10 +80,11 @@ export async function promptServiceName(kind: ServiceKind): Promise { } /** Prompts on a real terminal only — an agent's stdin is not one, and must never block. */ -export function serviceArgsDeps(): ServiceArgsDeps { +export function serviceArgsDeps(json?: boolean): ServiceArgsDeps { return { selectType: promptServiceType, askName: promptServiceName, - tty: !!process.stdin.isTTY && !!process.stdout.isTTY, + // --json asked for parseable output, so a caller that happens to own a TTY still gets the error. + tty: !json && !!process.stdin.isTTY && !!process.stdout.isTTY, } } diff --git a/test/resolve-service.test.ts b/test/resolve-service.test.ts index f7cc02f..a4f17e1 100644 --- a/test/resolve-service.test.ts +++ b/test/resolve-service.test.ts @@ -8,6 +8,7 @@ import { SERVICE_KINDS, missingArgsMessage, resolveServiceArgs, + serviceArgsDeps, serviceKindLines, type ServiceArgsDeps, } from '../src/resolve-service.js' @@ -60,6 +61,19 @@ test('unknown type: passed through for assertType to report, prompts untouched', expect(r).toEqual({ type: 'mysql', name: '' }) }) +// --json promises parseable stdout; a prompt would corrupt it and hang an agent that owns a TTY. +test('--json opts out of the prompts even on a terminal', () => { + const io = [process.stdin, process.stdout] as Array<{ isTTY?: boolean }> + const saved = io.map((s) => s.isTTY) + for (const s of io) s.isTTY = true + try { + expect(serviceArgsDeps().tty).toBe(true) + expect(serviceArgsDeps(true).tty).toBe(false) + } finally { + io.forEach((s, i) => { s.isTTY = saved[i] }) + } +}) + test('kind lines stay one per type and mention what each is', () => { const lines = serviceKindLines() expect(lines).toHaveLength(SERVICE_TYPES.length) From ed05552d47aff2f31c6e3df24241348e2c00ad79 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Thu, 13 Aug 2026 18:17:22 -0700 Subject: [PATCH 3/5] feat(services): lift Docker Image to its own kind, matching Add Service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prompt offered postgres / storage / compute, so running an existing container image was reachable only by already knowing the --image flag. The dashboard's Add Service menu lists Docker Image BESIDE Empty Service — a separate intent, not a compute flag — and the CLI now offers the same four kinds in the same order. Default names come from the same dialog's placeholders, so the two can't drift: main-db, assets, and compute (the CLI's compute default was `app`). Picking Docker Image asks for the ref, suggests a name derived from it with the dashboard's own rule (last path segment, sans tag/digest, kebab-safe), then the port with 8080 prefilled — a port mismatch is the first thing that makes a compute service unreachable, and whoever supplies an image knows what it listens on. A flag already on the command line is an answer: --image/--port are never asked for twice. Github Repo stays out of the list: the platform has no repo path yet, so a CLI entry could only say "coming soon". --- src/index.ts | 7 +- src/resolve-service.ts | 134 +++++++++++++++++++++++++++-------- test/resolve-service.test.ts | 94 ++++++++++++++++++------ 3 files changed, 184 insertions(+), 51 deletions(-) diff --git a/src/index.ts b/src/index.ts index 54f1201..c0b991c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -118,7 +118,8 @@ br.command('merge ').description('Merge a branch service set into anothe // ---- services (opt-in postgres/storage/compute) ---- const svc = program.command('services').alias('svc').description('Manage project services (postgres|storage|compute)') // [type] [name] are optional so the command can answer "what can I add?" — a terminal is walked -// through the two questions, anything else gets the kind list back as an error (resolve-service.ts). +// through the dashboard's Add Service kinds, anything else gets that list back as an error +// (resolve-service.ts). Picking Docker Image also fills in --image/--port from the answers. svc.command('add [type] [name]').description('Provision a service on demand (assigns a default domain for postgres/compute); with no type/name, a terminal picks from the service kinds') .option('--branch ', 'target branch (default: current)') .option('--region ', 'region for postgres/compute, e.g. us-east (see `insta regions`)') @@ -129,8 +130,8 @@ svc.command('add [type] [name]').description('Provision a service on demand (ass .option('--volume ', 'compute only: attach a persistent /data volume of this many whole Gi (also attachable later: `insta compute volume --size `; any plan may attach at the default 1; larger sizes are paid and plan-capped). Volume services keep 1 machine and stop (cold wake) instead of suspend when idle') .option('--json') .action(guard(async (type, name, o) => { - const a = await resolveServiceArgs(type, name, serviceArgsDeps(o.json)) - return services.servicesAdd(a.type, a.name, o) + const a = await resolveServiceArgs(type, name, serviceArgsDeps(o.json), o) + return services.servicesAdd(a.type, a.name, { ...o, image: a.image ?? o.image, port: a.port ?? o.port }) })) svc.command('list').option('--json').option('--branch ', 'branch (default: current)') .action(guard((o) => services.servicesList(o))) diff --git a/src/resolve-service.ts b/src/resolve-service.ts index 61f2ffc..94c3094 100644 --- a/src/resolve-service.ts +++ b/src/resolve-service.ts @@ -1,70 +1,132 @@ -// `insta services add` with no type (or no name): the three service kinds are otherwise only -// discoverable by guessing wrong and reading `type must be postgres|storage|compute`, so missing -// arguments answer "what can I add?" instead. A terminal gets the two questions the dashboard's -// Add Service asks — what, then what to call it; an agent gets the same list as an error, because -// nothing was created and a silent exit 0 would read as success. No flags are asked for: postgres -// and storage need none, and a compute service is legitimately empty until `insta deploy`. +// `insta services add` with no type (or no name): the kinds are otherwise only discoverable by +// guessing wrong and reading `type must be postgres|storage|compute`, so missing arguments answer +// "what can I add?" instead. The list mirrors the dashboard's Add Service menu (frontend +// `add-service-button.tsx`) — Docker Image sits BESIDE Empty Service, not under it, because +// picking an image is a different intent rather than a compute flag. An agent gets the same list +// as an error, because nothing was created and a silent exit 0 would read as success. import * as clack from '@clack/prompts' import { SERVICE_TYPES, assertServiceName, type ServiceType } from './commands/services.js' -export type ServiceKind = { type: ServiceType; hint: string; defaultName: string } +export type ServiceKind = { + id: string + label: string + type: ServiceType + hint: string + // Docker Image derives its name from the ref, so it carries no fixed default. + defaultName?: string + needsImage?: boolean +} +// Same order, labels and default names as the dashboard's Add Service menu. Github Repo is left +// out: the platform has no repo path yet, so a CLI entry could only say "coming soon". export const SERVICE_KINDS: readonly ServiceKind[] = [ - { type: 'postgres', hint: 'relational DB, usable as soon as it is added', defaultName: 'main-db' }, - { type: 'storage', hint: 'S3-compatible bucket, private by default', defaultName: 'assets' }, - { type: 'compute', hint: 'an app to deploy code to (empty until `insta deploy`)', defaultName: 'app' }, + { id: 'image', label: 'Docker Image', type: 'compute', hint: 'run an existing container image', needsImage: true }, + { id: 'postgres', label: 'Postgres', type: 'postgres', hint: 'relational DB, usable as soon as it is added', defaultName: 'main-db' }, + { id: 'storage', label: 'Storage', type: 'storage', hint: 'S3-compatible bucket, private by default', defaultName: 'assets' }, + { id: 'compute', label: 'Empty Service', type: 'compute', hint: 'an app to deploy code to (empty until `insta deploy`)', defaultName: 'compute' }, ] +// The platform's own default; the dialog prefills the same number. +export const DEFAULT_IMAGE_PORT = '8080' + +export type ResolvedServiceArgs = { type: string; name: string; image?: string; port?: string } + export type ServiceArgsDeps = { - selectType: (kinds: readonly ServiceKind[]) => Promise - askName: (kind: ServiceKind) => Promise + selectKind: (kinds: readonly ServiceKind[]) => Promise + askImage: () => Promise + askName: (kind: ServiceKind, suggested: string) => Promise + askPort: (fallback: string) => Promise tty: boolean } +/** Registry refs aren't URLs — quietly strip a pasted scheme prefix (mirrors the dashboard). */ +export function normalizeImageRef(raw: string): string { + return raw.trim().replace(/^https?:\/\//, '') +} + +/** Name from an image ref: last path segment, sans tag/digest, kebab-safe (mirrors the dashboard). */ +export function suggestServiceName(ref: string): string { + const last = ref.split('@')[0]!.split('/').pop() ?? '' + return last + .split(':')[0]! + .toLowerCase() + .replace(/[^a-z0-9-]+/g, '-') + .replace(/^-+|-+$/g, '') +} + +/** The non-interactive command for a kind — what an agent should run instead of being asked. */ +export function kindCommand(k: ServiceKind): string { + if (k.needsImage) return `insta services add compute --image --port ` + return `insta services add ${k.type} ${k.defaultName}` +} + /** The kind list, one line each — what a terminal picks from and an agent reads. */ export function serviceKindLines(): string[] { - return SERVICE_KINDS.map((k) => ` ${k.type.padEnd(9)} ${k.hint}`) + return SERVICE_KINDS.map((k) => ` ${k.label.padEnd(14)} ${kindCommand(k)}`) } /** What to say when there is no terminal to ask: the missing half, and how to supply it. */ export function missingArgsMessage(type?: string): string { - const known = SERVICE_KINDS.find((k) => k.type === type) - if (known) return `name the service: insta services add ${known.type} ${known.defaultName}` - return ['what to add:', ...serviceKindLines(), '', ' e.g. insta services add postgres main-db'].join('\n') + // A bare type names the plain kind, never Docker Image — that one is reached with --image. + const known = SERVICE_KINDS.find((k) => k.type === type && !k.needsImage) + if (known) return `name the service: ${kindCommand(known)}` + return ['what to add:', ...serviceKindLines()].join('\n') } /** * Fill in whatever `insta services add` was not given. An unknown type passes straight through so - * `assertType` — not this — reports it, keeping one wording for a bad type everywhere. + * `assertType` — not this — reports it, keeping one wording for a bad type everywhere. Flags that + * were already supplied are never asked for again. */ export async function resolveServiceArgs( type: string | undefined, name: string | undefined, deps: ServiceArgsDeps, -): Promise<{ type: string; name: string }> { + given: { image?: string; port?: string } = {}, +): Promise { if (type && name) return { type, name } if (type && !SERVICE_TYPES.includes(type as ServiceType)) return { type, name: name ?? '' } if (!deps.tty) throw new Error(missingArgsMessage(type)) - const picked = type ? (type as ServiceType) : await deps.selectType(SERVICE_KINDS) - const kind = SERVICE_KINDS.find((k) => k.type === picked) - if (!kind) return { type: picked, name: name ?? '' } - return { type: picked, name: name ?? (await deps.askName(kind)) } + const kind = type + ? SERVICE_KINDS.find((k) => k.type === type && !k.needsImage) + : await deps.selectKind(SERVICE_KINDS) + if (!kind) return { type: type!, name: name ?? '' } + if (!kind.needsImage) { + return { type: kind.type, name: name ?? (await deps.askName(kind, kind.defaultName ?? '')) } + } + const image = normalizeImageRef(given.image ?? (await deps.askImage())) + return { + type: kind.type, + name: name ?? (await deps.askName(kind, suggestServiceName(image))), + image, + port: given.port ?? (await deps.askPort(DEFAULT_IMAGE_PORT)), + } } /** Real prompts (clack, as the InsForge CLI's `create`); cancelling exits without provisioning. */ -export async function promptServiceType(kinds: readonly ServiceKind[]): Promise { +export async function promptServiceKind(kinds: readonly ServiceKind[]): Promise { const picked = await clack.select({ message: 'What do you want to add?', - options: kinds.map((k) => ({ value: k.type, label: k.type, hint: k.hint })), + options: kinds.map((k) => ({ value: k.id, label: k.label, hint: k.hint })), }) if (clack.isCancel(picked)) process.exit(0) - return picked + return SERVICE_KINDS.find((k) => k.id === picked)! } -export async function promptServiceName(kind: ServiceKind): Promise { +export async function promptImageRef(): Promise { + const answer = await clack.text({ + message: 'Image reference:', + placeholder: 'nginx:latest', + validate: (v) => (normalizeImageRef(v) ? undefined : 'an image reference is required'), + }) + if (clack.isCancel(answer)) process.exit(0) + return answer +} + +export async function promptServiceName(kind: ServiceKind, suggested: string): Promise { const answer = await clack.text({ message: `Name this ${kind.type} service:`, - initialValue: kind.defaultName, + initialValue: suggested, // The same rule the command enforces, reported before Enter rather than after a round trip. validate: (v) => { try { @@ -79,11 +141,27 @@ export async function promptServiceName(kind: ServiceKind): Promise { return answer.trim() } +export async function promptPort(fallback: string): Promise { + const answer = await clack.text({ + message: 'Port the image listens on:', + initialValue: fallback, + // Mirrors the dialog's range check, so a typo is caught before the service is provisioned. + validate: (v) => { + const n = Number(v.trim()) + return Number.isInteger(n) && n >= 1 && n <= 65535 ? undefined : 'port must be an integer between 1 and 65535' + }, + }) + if (clack.isCancel(answer)) process.exit(0) + return answer.trim() +} + /** Prompts on a real terminal only — an agent's stdin is not one, and must never block. */ export function serviceArgsDeps(json?: boolean): ServiceArgsDeps { return { - selectType: promptServiceType, + selectKind: promptServiceKind, + askImage: promptImageRef, askName: promptServiceName, + askPort: promptPort, // --json asked for parseable output, so a caller that happens to own a TTY still gets the error. tty: !json && !!process.stdin.isTTY && !!process.stdout.isTTY, } diff --git a/test/resolve-service.test.ts b/test/resolve-service.test.ts index a4f17e1..46a0ac0 100644 --- a/test/resolve-service.test.ts +++ b/test/resolve-service.test.ts @@ -1,28 +1,52 @@ // `insta services add` used to answer a missing type with commander's "missing required argument", // which never says what the types are. Resolution: both args given → untouched (no prompt anywhere -// near the fast path); TTY → ask what, then what to call it; no TTY → the kind list as an error, -// because nothing was created; a bad type → straight through, so assertType keeps owning that -// wording. +// near the fast path); TTY → the dashboard's Add Service kinds, then a name (and for Docker Image, +// the ref first and the port after); no TTY → the kind list as an error, because nothing was +// created; a bad type → straight through, so assertType keeps owning that wording. import { test, expect } from 'vitest' import { + DEFAULT_IMAGE_PORT, SERVICE_KINDS, missingArgsMessage, + normalizeImageRef, resolveServiceArgs, serviceArgsDeps, serviceKindLines, + suggestServiceName, type ServiceArgsDeps, + type ServiceKind, } from '../src/resolve-service.js' import { SERVICE_TYPES } from '../src/commands/services.js' +const kind = (id: string): ServiceKind => SERVICE_KINDS.find((k) => k.id === id)! + const deps = (over: Partial = {}): ServiceArgsDeps => ({ - selectType: async () => { throw new Error('selectType must not be called') }, + selectKind: async () => { throw new Error('selectKind must not be called') }, + askImage: async () => { throw new Error('askImage must not be called') }, askName: async () => { throw new Error('askName must not be called') }, + askPort: async () => { throw new Error('askPort must not be called') }, tty: true, ...over, }) -test('every service type has a kind entry to offer', () => { - expect(SERVICE_KINDS.map((k) => k.type)).toEqual([...SERVICE_TYPES]) +test('every service type is reachable from some kind', () => { + for (const t of SERVICE_TYPES) expect(SERVICE_KINDS.some((k) => k.type === t)).toBe(true) +}) + +// The dashboard's Add Service lists Docker Image beside Empty Service, not under it. +test('Docker Image is its own kind, at the same level as Empty Service', () => { + expect(SERVICE_KINDS.map((k) => k.label)).toEqual(['Docker Image', 'Postgres', 'Storage', 'Empty Service']) + expect(kind('image').needsImage).toBe(true) + expect(kind('image').type).toBe('compute') + expect(kind('compute').needsImage).toBeUndefined() +}) + +// Default names are the dashboard dialog's placeholders — they must not drift apart. +test('default names match the Add Service placeholders', () => { + expect(kind('postgres').defaultName).toBe('main-db') + expect(kind('storage').defaultName).toBe('assets') + expect(kind('compute').defaultName).toBe('compute') + expect(kind('image').defaultName).toBeUndefined() }) test('both arguments given: returned as-is, nothing is asked', async () => { @@ -33,23 +57,45 @@ test('both arguments given: returned as-is, nothing is asked', async () => { test('no arguments + TTY: asks what, then the name for that kind', async () => { const asked: string[] = [] const r = await resolveServiceArgs(undefined, undefined, deps({ - selectType: async (kinds) => { asked.push('type'); return kinds[1]!.type }, - askName: async (kind) => { asked.push(`name:${kind.type}`); return kind.defaultName }, + selectKind: async (kinds) => { asked.push('kind'); return kinds.find((k) => k.id === 'storage')! }, + askName: async (k, suggested) => { asked.push(`name:${k.id}`); return suggested }, })) expect(r).toEqual({ type: 'storage', name: 'assets' }) - expect(asked).toEqual(['type', 'name:storage']) + expect(asked).toEqual(['kind', 'name:storage']) }) -test('type given, name missing + TTY: only the name is asked', async () => { - const r = await resolveServiceArgs('compute', undefined, deps({ askName: async (k) => k.defaultName })) - expect(r).toEqual({ type: 'compute', name: 'app' }) +test('Docker Image: asks for the ref, suggests a name from it, then the port', async () => { + const asked: string[] = [] + const r = await resolveServiceArgs(undefined, undefined, deps({ + selectKind: async () => { asked.push('kind'); return kind('image') }, + askImage: async () => { asked.push('image'); return 'ghcr.io/insforge/postgres:v15.13.4' }, + askName: async (_k, suggested) => { asked.push('name'); return suggested }, + askPort: async (fallback) => { asked.push('port'); return fallback }, + })) + expect(r).toEqual({ type: 'compute', name: 'postgres', image: 'ghcr.io/insforge/postgres:v15.13.4', port: DEFAULT_IMAGE_PORT }) + expect(asked).toEqual(['kind', 'image', 'name', 'port']) +}) + +// A flag already on the command line is an answer — asking for it again would be a regression. +test('Docker Image: --image and --port already given are not asked for', async () => { + const r = await resolveServiceArgs(undefined, undefined, deps({ + selectKind: async () => kind('image'), + askName: async (_k, suggested) => suggested, + }), { image: 'https://nginx:1.27', port: '3000' }) + expect(r).toEqual({ type: 'compute', name: 'nginx', image: 'nginx:1.27', port: '3000' }) }) -test('no TTY: throws, and the message lists every kind with an example', async () => { +test('a bare compute type means Empty Service, never the image flow', async () => { + const r = await resolveServiceArgs('compute', undefined, deps({ askName: async (_k, s) => s })) + expect(r).toEqual({ type: 'compute', name: 'compute' }) +}) + +test('no TTY: throws, and the message lists every kind with its command', async () => { await expect(resolveServiceArgs(undefined, undefined, deps({ tty: false }))).rejects.toThrow(/what to add/) const msg = missingArgsMessage() - for (const t of SERVICE_TYPES) expect(msg).toContain(t) + for (const k of SERVICE_KINDS) expect(msg).toContain(k.label) expect(msg).toContain('insta services add postgres main-db') + expect(msg).toContain('--image ') }) test('no TTY with a type: asks for the missing half, not the whole list', () => { @@ -61,6 +107,20 @@ test('unknown type: passed through for assertType to report, prompts untouched', expect(r).toEqual({ type: 'mysql', name: '' }) }) +test('kind lines stay one per kind and carry a runnable command', () => { + const lines = serviceKindLines() + expect(lines).toHaveLength(SERVICE_KINDS.length) + expect(lines.join('\n')).toContain('insta services add storage assets') +}) + +// Same rules as the dashboard's helpers, so a ref names the service identically in both. +test('image refs normalize and suggest the dashboard name', () => { + expect(normalizeImageRef(' https://ghcr.io/insforge/app:v2 ')).toBe('ghcr.io/insforge/app:v2') + expect(suggestServiceName('nginx:latest')).toBe('nginx') + expect(suggestServiceName('ghcr.io/insforge/postgres-all:latest')).toBe('postgres-all') + expect(suggestServiceName('registry.io/team/My_App@sha256:abc')).toBe('my-app') +}) + // --json promises parseable stdout; a prompt would corrupt it and hang an agent that owns a TTY. test('--json opts out of the prompts even on a terminal', () => { const io = [process.stdin, process.stdout] as Array<{ isTTY?: boolean }> @@ -73,9 +133,3 @@ test('--json opts out of the prompts even on a terminal', () => { io.forEach((s, i) => { s.isTTY = saved[i] }) } }) - -test('kind lines stay one per type and mention what each is', () => { - const lines = serviceKindLines() - expect(lines).toHaveLength(SERVICE_TYPES.length) - expect(lines.join('\n')).toContain('empty until `insta deploy`') -}) From fd3bdccb83050513e30b174035f5103c0d6bae59 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Thu, 13 Aug 2026 20:49:16 -0700 Subject: [PATCH 4/5] fix(services): reject empty image refs and out-of-range ports (cubic) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, all reachable through the new Docker Image kind: - `--image` that normalizes to nothing (`https://`, whitespace) skipped the prompt's validator and provisioned a plain empty compute instead, because servicesAddRequestBody drops a falsy image. Rejected before the name is asked. - A repo segment over 39 chars produced a suggested name assertServiceName rejects, so it could not be accepted unchanged. Capped, with no trailing hyphen left behind. - `--port` was never range-checked anywhere: `Number('abc')` reached the API as NaN, which serializes to null. New `parsePort` beside parseCount / parseVolumeGib is now the single rule — servicesAdd validates ahead of any network access, the request body uses it, and the prompt's validator calls it so a typed port and a --port can never disagree. A bad --port fails before the first question rather than after three. --- src/commands/services.ts | 14 ++++++++++++-- src/resolve-service.ts | 25 ++++++++++++++++++++----- test/resolve-service.test.ts | 27 ++++++++++++++++++++++++++- test/services.test.ts | 17 ++++++++++++++++- 4 files changed, 74 insertions(+), 9 deletions(-) diff --git a/src/commands/services.ts b/src/commands/services.ts index 9717b7f..358d8d4 100644 --- a/src/commands/services.ts +++ b/src/commands/services.ts @@ -28,6 +28,13 @@ export function parseCount(raw: string): number { return n } +// Parse a TCP port. Junk fails here rather than reaching the API as NaN (the parseCpu lesson). +export function parsePort(raw: string): number { + const n = Number(raw) + if (!Number.isInteger(n) || n < 1 || n > 65535) throw new Error(`port must be an integer between 1 and 65535, got: ${raw}`) + return n +} + // Parse a volume size in whole Gi: "10" or "10Gi" (suffix case-insensitive — unlike the db // quantity strings this is not a provider pass-through; the wire value is an integer). Volumes // are provisioned block disks, so fractional and Mi values are rejected locally with an example @@ -70,7 +77,7 @@ export type ServicesAddOpts = { branch?: string; public?: boolean; image?: strin export function servicesAddRequestBody(type: string, name: string, branch: string | undefined, opts: ServicesAddOpts): Record { return { type, name, ...(branch ? { branch } : {}), public: !!opts.public, - ...(opts.image ? { image: opts.image } : {}), ...(opts.port ? { port: Number(opts.port) } : {}), + ...(opts.image ? { image: opts.image } : {}), ...(opts.port ? { port: parsePort(opts.port) } : {}), ...(opts.region ? { region: opts.region } : {}), ...(opts.alwaysOn ? { alwaysOn: true } : {}), ...(opts.volume !== undefined ? { volumeGib: parseVolumeGib(opts.volume) } : {}), @@ -82,7 +89,10 @@ export async function servicesAdd(type: string, name: string, opts: ServicesAddO if (opts.public && type !== 'storage') throw new Error('--public is only valid for storage services') if (opts.region && type === 'storage') throw new Error('--region is not valid for storage services') if (opts.image && type !== 'compute') throw new Error('--image is only valid for compute services') - if (opts.port && type !== 'compute') throw new Error('--port is only valid for compute services') + if (opts.port) { + if (type !== 'compute') throw new Error('--port is only valid for compute services') + parsePort(opts.port) // junk fails here, before any config/network access + } if (opts.alwaysOn && type !== 'compute') throw new Error('--always-on is only valid for compute services (for postgres, use `insta db always-on on` after creation)') if (opts.volume !== undefined) { if (type !== 'compute') throw new Error('--volume is only valid for compute services (postgres has one by default — grow it with `insta db volume --size`)') diff --git a/src/resolve-service.ts b/src/resolve-service.ts index 94c3094..61cba08 100644 --- a/src/resolve-service.ts +++ b/src/resolve-service.ts @@ -5,7 +5,7 @@ // picking an image is a different intent rather than a compute flag. An agent gets the same list // as an error, because nothing was created and a silent exit 0 would read as success. import * as clack from '@clack/prompts' -import { SERVICE_TYPES, assertServiceName, type ServiceType } from './commands/services.js' +import { SERVICE_TYPES, assertServiceName, parsePort, type ServiceType } from './commands/services.js' export type ServiceKind = { id: string @@ -44,7 +44,11 @@ export function normalizeImageRef(raw: string): string { return raw.trim().replace(/^https?:\/\//, '') } -/** Name from an image ref: last path segment, sans tag/digest, kebab-safe (mirrors the dashboard). */ +/** + * Name from an image ref: last path segment, sans tag/digest, kebab-safe (the dashboard's rule). + * Also capped at the 39 chars `assertServiceName` allows — a suggestion the user cannot accept + * unchanged is worse than none. + */ export function suggestServiceName(ref: string): string { const last = ref.split('@')[0]!.split('/').pop() ?? '' return last @@ -52,6 +56,8 @@ export function suggestServiceName(ref: string): string { .toLowerCase() .replace(/[^a-z0-9-]+/g, '-') .replace(/^-+|-+$/g, '') + .slice(0, 39) + .replace(/-+$/g, '') } /** The non-interactive command for a kind — what an agent should run instead of being asked. */ @@ -87,6 +93,8 @@ export async function resolveServiceArgs( if (type && name) return { type, name } if (type && !SERVICE_TYPES.includes(type as ServiceType)) return { type, name: name ?? '' } if (!deps.tty) throw new Error(missingArgsMessage(type)) + // A bad --port is a typo in the command, not an answer: fail before asking anything. + if (given.port !== undefined) parsePort(given.port) const kind = type ? SERVICE_KINDS.find((k) => k.type === type && !k.needsImage) : await deps.selectKind(SERVICE_KINDS) @@ -94,7 +102,10 @@ export async function resolveServiceArgs( if (!kind.needsImage) { return { type: kind.type, name: name ?? (await deps.askName(kind, kind.defaultName ?? '')) } } + // The prompt validates a typed ref; a --image that normalizes away would slip past it and + // provision a plain empty compute instead (servicesAddRequestBody drops a falsy image). const image = normalizeImageRef(given.image ?? (await deps.askImage())) + if (!image) throw new Error('an image reference is required') return { type: kind.type, name: name ?? (await deps.askName(kind, suggestServiceName(image))), @@ -145,10 +156,14 @@ export async function promptPort(fallback: string): Promise { const answer = await clack.text({ message: 'Port the image listens on:', initialValue: fallback, - // Mirrors the dialog's range check, so a typo is caught before the service is provisioned. + // The rule the command enforces, so the prompt and a --port can never disagree. validate: (v) => { - const n = Number(v.trim()) - return Number.isInteger(n) && n >= 1 && n <= 65535 ? undefined : 'port must be an integer between 1 and 65535' + try { + parsePort(v.trim()) + return undefined + } catch (e) { + return (e as Error).message + } }, }) if (clack.isCancel(answer)) process.exit(0) diff --git a/test/resolve-service.test.ts b/test/resolve-service.test.ts index 46a0ac0..ff16fa5 100644 --- a/test/resolve-service.test.ts +++ b/test/resolve-service.test.ts @@ -16,7 +16,7 @@ import { type ServiceArgsDeps, type ServiceKind, } from '../src/resolve-service.js' -import { SERVICE_TYPES } from '../src/commands/services.js' +import { SERVICE_TYPES, assertServiceName } from '../src/commands/services.js' const kind = (id: string): ServiceKind => SERVICE_KINDS.find((k) => k.id === id)! @@ -121,6 +121,31 @@ test('image refs normalize and suggest the dashboard name', () => { expect(suggestServiceName('registry.io/team/My_App@sha256:abc')).toBe('my-app') }) +// A suggestion the name rule would reject is worse than none — it can't be accepted unchanged. +test('a long repo segment is capped at what assertServiceName accepts', () => { + const suggested = suggestServiceName(`ghcr.io/org/${'a'.repeat(50)}:latest`) + expect(suggested).toHaveLength(39) + expect(() => assertServiceName(suggested)).not.toThrow() + // Truncation must not leave a trailing hyphen, which the rule also rejects. + expect(suggestServiceName(`ghcr.io/org/${'ab-'.repeat(20)}:latest`)).not.toMatch(/-$/) +}) + +// --image that normalizes away would otherwise be dropped from the body and quietly build an +// empty compute service instead of the image the user asked for. +test('an --image that normalizes to nothing is rejected, not silently dropped', async () => { + await expect(resolveServiceArgs(undefined, undefined, deps({ + selectKind: async () => kind('image'), + }), { image: 'https://' })).rejects.toThrow(/image reference is required/) +}) + +// A bad --port is a typo in the command; answering three questions first would be wasted work. +test('an invalid --port fails before any prompt', async () => { + await expect(resolveServiceArgs(undefined, undefined, deps(), { port: '70000' })) + .rejects.toThrow(/between 1 and 65535/) + await expect(resolveServiceArgs(undefined, undefined, deps(), { port: 'abc' })) + .rejects.toThrow(/between 1 and 65535/) +}) + // --json promises parseable stdout; a prompt would corrupt it and hang an agent that owns a TTY. test('--json opts out of the prompts even on a terminal', () => { const io = [process.stdin, process.stdout] as Array<{ isTTY?: boolean }> diff --git a/test/services.test.ts b/test/services.test.ts index 74da71f..d5e4be3 100644 --- a/test/services.test.ts +++ b/test/services.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest' import { - assertType, assertServiceName, parseCount, parseAccess, resolveServiceId, resolveComputeServiceId, SERVICE_TYPES, + assertType, assertServiceName, parseCount, parsePort, parseAccess, resolveServiceId, resolveComputeServiceId, SERVICE_TYPES, servicesAddRequestBody, servicesAdd, serviceListLine, } from '../src/commands/services.js' @@ -31,6 +31,21 @@ describe('parseCount', () => { }) }) +describe('parsePort', () => { + it('parses ports in range', () => { + expect(parsePort('8080')).toBe(8080) + expect(parsePort('1')).toBe(1) + expect(parsePort('65535')).toBe(65535) + }) + // Junk used to reach the API as NaN, which serializes to null. + it('rejects out-of-range and non-integer ports', () => { + expect(() => parsePort('0')).toThrow(/between 1 and 65535/) + expect(() => parsePort('65536')).toThrow(/between 1 and 65535/) + expect(() => parsePort('8080.5')).toThrow(/between 1 and 65535/) + expect(() => parsePort('abc')).toThrow(/between 1 and 65535/) + }) +}) + describe('assertServiceName', () => { it('accepts lower-kebab service names', () => { expect(() => assertServiceName('primary-db')).not.toThrow() From 61199962fbbc694197c6aa62ca05d94bf238bcee Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Thu, 13 Aug 2026 20:59:34 -0700 Subject: [PATCH 5/5] fix(services): decimal-only ports, and resolve the kind from the list shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P3 review findings. parsePort took whatever Number() would coerce, so 0x1f90 passed as 8080 and 1e3 as 1000 while the error string promised a decimal integer. A port written in hex is a typo worth reporting, not one worth honouring — digits only now, as parseVolumeGib. Surrounding whitespace stays tolerated: that is shell noise rather than a mistake, and parseVolumeGib allows it too. promptServiceKind rendered the `kinds` it was handed but resolved the answer against the module-level SERVICE_KINDS, so the parameter was decorative and the non-null assertion hid it. Nothing can reach it today (the only caller passes the registry), but any filtered list — a plan-gated subset, say — would return undefined as a ServiceKind. It now looks the id up in the list it displayed, which is the contract the tests' fake already assumes. --- src/commands/services.ts | 5 ++++- src/resolve-service.ts | 3 ++- test/services.test.ts | 11 +++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/commands/services.ts b/src/commands/services.ts index 358d8d4..f62ebcc 100644 --- a/src/commands/services.ts +++ b/src/commands/services.ts @@ -29,8 +29,11 @@ export function parseCount(raw: string): number { } // Parse a TCP port. Junk fails here rather than reaching the API as NaN (the parseCpu lesson). +// Decimal digits only, as parseVolumeGib: `Number()` alone would quietly read 0x1f90 as 8080 and +// 1e3 as 1000, and a port written in hex is a typo worth reporting, not one worth honouring. export function parsePort(raw: string): number { - const n = Number(raw) + const m = /^\s*(\d+)\s*$/.exec(raw) + const n = m ? Number(m[1]) : NaN if (!Number.isInteger(n) || n < 1 || n > 65535) throw new Error(`port must be an integer between 1 and 65535, got: ${raw}`) return n } diff --git a/src/resolve-service.ts b/src/resolve-service.ts index 61cba08..8b5b378 100644 --- a/src/resolve-service.ts +++ b/src/resolve-service.ts @@ -121,7 +121,8 @@ export async function promptServiceKind(kinds: readonly ServiceKind[]): Promise< options: kinds.map((k) => ({ value: k.id, label: k.label, hint: k.hint })), }) if (clack.isCancel(picked)) process.exit(0) - return SERVICE_KINDS.find((k) => k.id === picked)! + // Resolve against the list that was displayed — a subset must not fall through to the registry. + return kinds.find((k) => k.id === picked)! } export async function promptImageRef(): Promise { diff --git a/test/services.test.ts b/test/services.test.ts index d5e4be3..222933d 100644 --- a/test/services.test.ts +++ b/test/services.test.ts @@ -44,6 +44,17 @@ describe('parsePort', () => { expect(() => parsePort('8080.5')).toThrow(/between 1 and 65535/) expect(() => parsePort('abc')).toThrow(/between 1 and 65535/) }) + // Number() would read these as 8080 and 1000 — a port in hex is a typo, not a port. + it('rejects non-decimal spellings Number() would have accepted', () => { + expect(() => parsePort('0x1f90')).toThrow(/between 1 and 65535/) + expect(() => parsePort('1e3')).toThrow(/between 1 and 65535/) + expect(() => parsePort('0o17620')).toThrow(/between 1 and 65535/) + expect(() => parsePort('')).toThrow(/between 1 and 65535/) + }) + // Surrounding whitespace is shell noise, not a typo — parseVolumeGib tolerates it too. + it('tolerates surrounding whitespace', () => { + expect(parsePort(' 8080 ')).toBe(8080) + }) }) describe('assertServiceName', () => {