feat(services): answer a bare services add with the kinds, and give it --json - #91
Conversation
`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.
jwfing
left a comment
There was a problem hiding this comment.
Review: feat(services): answer 'services add' with no type by asking what to add
Summary: A tight, well-tested UX improvement that turns insta services add's missing-argument error into a guided prompt (TTY) or a discoverable kind-list error (non-TTY); the fully-specified path and servicesAdd's signature are genuinely untouched.
Requirements context
No /docs/superpowers/ or docs/specs/ directory exists in this repo (confirmed via workspace) — no matching spec/plan, so I assessed against the PR description, AGENTS.md, and the existing code conventions. The change is squarely in scope: it only adds argument resolution ahead of the unchanged servicesAdd.
Findings
Critical
(none) — logic, tests, security, and performance all check out. Notably the resolution order is correct: unknown types pass straight through (resolve-service.ts:41) and both-args-given returns before any TTY/prompt logic (resolve-service.ts:40), so assertType (src/commands/services.ts:81, first line of servicesAdd) remains the single place that words a bad type and no empty name can reach the API. The new async action composes cleanly with guard (src/index.ts:35), which already .catch(onError)s. clack API usage (select/text/isCancel, option {value,label,hint}, validate returning string|undefined) verified against @clack/prompts 0.9.x docs — no stale/hallucinated signatures.
Suggestion
- Software engineering / consistency — new runtime dep vs. the repo's own precedent (
package.json:45,src/resolve-service.ts:7): this adds@clack/prompts(+ transitive@clack/core,sisteransi) as the CLI's first interactive-prompt dependency, whereas the existing in-repo pickersrc/resolve-project.tsis deliberately dependency-free (node:readline/promises, see itspromptChoice). The PR justifies clack as "matching the InsForge CLI'screateflow," but that precedent is the cloud CLI, not this repo — withininsta-clithe established pattern is readline. Not blocking (dep is reputable/bombshell-dev, lockfile-pinned with integrity hashes, and the injected-deps design keeps it out of the tested core), but worth a conscious decision rather than drift. - Test coverage — the real prompt wrappers are untested (
src/resolve-service.ts:52-83):resolveServiceArgsis thoroughly unit-tested via injected deps (good), butpromptServiceType/promptServiceName— including thevalidateclosure that funnelsassertServiceNameerrors into the prompt — have no automated coverage (only the manual pty run described in the PR). Low blast radius, and hard to unit-test without a TTY harness, so a Suggestion rather than a gap.
Information
- Cancel exits 0, non-TTY missing-args exits 1 (
src/resolve-service.ts:60,73vs:41):process.exit(0)onisCancelis the clack convention and is intentional (user aborted vs. a script hitting empty vars), but note the two "didn't provision" paths deliberately use different exit codes. Also, callingprocess.exitdirectly from this otherwise-pure module is the one spot that couples it to process lifecycle — fine, just flagging. - Name validation is asymmetric between the two paths (pre-existing) (
src/commands/services.ts:80-90): the new interactive prompt validates the name viaassertServiceName, but the direct-arg path (insta services add postgres SomeName) still does not —servicesAddnever callsassertServiceName, relying on the server. This PR doesn't regress that; it just makes the guided path stricter than the flag path. Consider validating inservicesAddin a follow-up so the rule can't drift, but out of scope here. - AGENTS.md rule 4 follow-up (command signature changed
add <type> <name>→add [type] [name]): rule 4 requires mirroring command/flag changes inskills/insta/cli-reference.md. The PR explicitly defers this to a separate PR in theskillssubmodule — acknowledged; just make sure that lands so the agent-facing surface doc stays accurate.
Verdict
approved (informational — a human still gives the GitHub approval). Zero Critical findings; the two Suggestions and the Information notes are non-blocking. Clean separation of pure/testable resolution from the TTY prompts, correct pass-through to assertType, and no security/performance concerns (no new user input reaching SQL/shell, no hot-path or I/O changes).
…readable `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.
services add with no type by asking what to addservices add with the kinds, and give it --json
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".
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… shown 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.
What
insta services addwith no arguments answerederror: missing required argument 'type'. The three kinds a project can run were discoverable only by guessing one wrong and readingassertType'stype must be postgres|storage|compute.Now the missing arguments are the question:
Fully-specified invocations are untouched —
insta services add postgres main-dbnever reaches a prompt.The same commit set also gives
adda--json, which it was the onlyservicescommand to lack:list,rename,set-access,scaleandupgradeall take one, so an agent that had just created a service still had to re-queryservices list --jsonto learn the id and domain it was given.removedeliberately stays without it — it destroys rather than produces, matchingbranch delete/project delete.How
src/resolve-service.ts(new):SERVICE_KINDS(type + one-line hint + default name),resolveServiceArgs()filling in whatever was not given, and the clack prompts behind injectable deps — the shaperesolve-project.tsalready uses for its project picker, so the logic is unit-testable without a TTY.src/index.ts:add <type> <name>→add [type] [name], and the action resolves the pair before callingservicesAdd.servicesAdd's signature and every existing test are unchanged; the dependency runs one way (resolve-service → services), so there is no import cycle.project createreturns 0 in the same spot because it has a fallback path (the agent skill); this has none.servicesAdd(src/commands/services.ts:80-101) every compute flag is optional — an empty compute is a legitimate service untilinsta deploy.--publicalso stays out of the flow: private is the safe default andinsta services set-accessflips it later.assertTyperemains the single place that words a bad type.@clack/prompts(+1 runtime dep, alongside commander), matching the InsForge CLI'screateflow — select, then text withinitialValueand inlinevalidate,isCancelexiting without provisioning. Name validation calls the command's ownassertServiceName, so the rule can't drift.--jsonprintsres.body.serviceright after the approval gate — the same shape and position asrename/set-access/scale/upgrade.serviceArgsDeps(json?)forcestty: falsewhen it is set, so a caller that asked for parseable stdout takes the kind-list error instead of a question that would corrupt the output and hang an agent that happens to own a TTY.Verify
npm run typecheckclean;npm test27 files / 247 tests pass (was 26 / 238 — 9 new, none changed).Built and exercised the real binary. No TTY: no args → kind list,
exit 1;services add storage→name the service: insta services add storage assets,exit 1;services add mysql foo→type must be postgres|storage|compute. None of these reach the API — resolution throws beforeApiClient.load().Drove the TTY path under a pty with
expect(prompts only, no provisioning): Enter →type=postgres name=main-db; ↓ then Enter →type=storage name=assets; typingBad_Nameis rejected inline with thelower-kebabmessage.--json:services add --helplists it;services add --jsonwith no positionals prints the kind list as an error and creates nothing (resolution still throws beforeApiClient.load()); new unit test--json opts out of the prompts even on a terminalpins the TTY opt-out.Doc mirror required by AGENTS.md rule 4 (
skills/insta/cli-reference.md,skillssubmodule): InsForge/insta-skills#38, which carries both the[type] [name]signature and--json.Summary by cubic
Guides
insta services addwhen type/name are omitted and adds--jsonfor machine‑readable output. Previously a missing type errored; now a TTY picks a kind and name, non‑TTY gets a kind list and exits non‑zero; created services can be printed as JSON.add [type] [name]with a new “Docker Image” kind. When chosen, asks for an image ref (scheme stripped, must be non‑empty), suggests a kebab‑case name capped at 39 chars, and asks for the port (default 8080). Provided flags (--image,--port) are not re‑asked. Adds--json.resolve-service.tswithSERVICE_KINDSand TTY‑gated prompts via@clack/prompts.--jsondisables prompts even on a TTY. Selection now resolves against the list shown, not a module registry, so filtered lists behave correctly.--jsonprints the created service object; human summary is unchanged. Cancellation exits 0 without provisioning. Non‑interactive missing args throw with a concise message listing kinds, or ask for the name when a valid type is given.--portuses a single rule that requires a decimal integer 1–65535 and is checked before any config/network access; the prompt uses the same rule. An--imagethat normalizes to empty is rejected. Unit tests cover resolution paths, Docker Image flow, name suggestion cap, decimal‑only port validation, prompt opt‑out with--json, and resolving the picked kind from the displayed list.Written for commit 6119996. Summary will update on new commits.