feat: compute volume --delete — destroy the /data volume

[tonychang04]

What

insta compute volume <service> --delete — DELETE .../volume through the same verb that attaches and grows. Destroys the disk and ALL its data immediately (no detach, no undo; billing stops now; suspend fast-wake + scale-out return).

• Backend messages speak verbatim as always: no volume = its 404, a governance policy answers 202 approval like any destructive verb (gated service.remove server-side).
• Older-backend fallback: a bare route-404 maps through the pure exported volumeDeleteError helper to "this backend does not support volume delete yet — update the platform" instead of parroting HTTP 404 (all three cases unit-pinned; the 404-with-body case verified live against the platform branch).
• --delete + --size conflict locally — one changes the volume, the other destroys it.

Required companion doc (AGENTS.md: mirror flag changes in skills/insta/cli-reference.md)

That file lives in the insta-skills repo, not in insta-cli — the update is InsForge/insta-skills#35: the insta compute volume row gains --delete and the Volumes section documents destroy semantics, billing stop, and the lifted constraints. Reviewed clean by both bots. The two PRs merge together.

Tests

237/237 — delete-line renderer, read-line delete hint, flag-conflict validation, and the three volumeDeleteError mapping cases. Per repo convention the suite pins pure seams (renderers + error mapping), not network orchestration; the full wire path (DELETE → read-back flip → re-attach, plus the 202 shape) is exercised end-to-end by the cleanroom leg in InsForge/insta-e2e#16.

Companions

insta-platform#189 · insta-frontend#123 · insta-e2e#16 · insta-skills#35

🤖 Generated with Claude Code

https://claude.ai/code/session_017eXAG6MxyFt5YMf22JdxJX

[jwfing]

Summary
This PR correctly adds insta compute volume --delete to the existing volume command surface, with a sensible older-backend fallback and local flag-conflict validation.

Requirements context
I based the intended behavior on the PR description and the local repo guidance in AGENTS.md and .claude/skills/developing-insta-cli/SKILL.md. Those docs establish that this CLI is a thin wrapper over backend API behavior, that destructive actions may return approval-required 202 responses, and that command/flag changes should be mirrored in skills/insta/cli-reference.md in the superproject. I could not run Vitest locally because this checkout does not have vitest installed (npm test -- test/volume.test.ts failed with sh: vitest: not found).

Findings

Critical
(none)

Suggestion

• test/volume.test.ts:107 and src/commands/compute.ts:170: the new tests only cover the local --delete/--size conflict and pure output rendering. They do not exercise the actual DELETE branch’s important behavior: successful delete output, 202 approval handling, or the bare-route-404 compatibility remap. That leaves the most behaviorally significant part of the feature unguarded.

Information

• src/index.ts:185, AGENTS.md:16, .claude/skills/developing-insta-cli/SKILL.md:35: this PR changes the CLI surface by adding --delete, but the required companion skills/insta/cli-reference.md update is not visible in this checkout. If that change lives in a companion PR, it would be worth linking it explicitly from this PR.
• src/commands/compute.ts:162: no security-relevant changes stood out here. The new path still goes through the existing typed API client, does not introduce new shell/SQL surfaces, and preserves backend-authored error messages and approval gating.
• src/commands/compute.ts:162: no performance concerns stood out here. The change adds one conditional DELETE branch on an already interactive CLI path and does not add new loops, synchronous hot-path work, or larger data fetches.

Verdict
Approved. The implementation matches the stated feature intent and follows the repo’s existing command/error-handling conventions; the remaining issues are non-blocking coverage/documentation gaps rather than merge blockers.

[jwfing]

Summary

Adds insta compute volume <service> --delete (DELETE .../volume) to permanently destroy a service's /data disk; the change is small, correct, well-scoped, and consistent with the CLI's existing destructive-verb conventions.

Requirements context

No matching spec/plan found — insta-cli has no docs/superpowers/ (or any docs/) directory, so this review assesses against the PR description and the surrounding code conventions alone. Verified against the sibling destructive commands in the repo (servicesRemove, projectDelete, branchDelete, the volume --size PUT path) and the ApiClient contract in src/api.ts.

Findings

Critical

(none)

Suggestion

• Software engineering — the new non-trivial branch is untested (src/commands/compute.ts:170-185). The three added tests cover the pure renderer (volumeDeleteLine), the read-hint string, and the flag-conflict guard — all good. But the most subtle new logic, the older-backend fallback (e instanceof ApiError && e.status === 404 && /^HTTP 404$/.test(e.message) → "update the platform"), has no test. This is exactly the kind of close-call branch (bare route-404 vs. a 404-with-body from a supporting backend) where a regression would slip through silently. Consider extracting the error-mapping into a small pure helper alongside volumeDeleteLine and unit-testing both the "bare 404 → hint" and "404-with-body → rethrow verbatim" cases. Non-blocking — it matches the repo's convention of testing pure renderers over network orchestration.

• Functionality — the 404 fallback conflates "route missing" with "no volume" iff a supporting backend ever returns a bodyless 404 (src/commands/compute.ts:177). The mapping is correct given the documented contract (a backend that has the route names the real problem in an error body, so e.message !== "HTTP 404"). If the companion platform (feat/volume-remove #189) ever answers "this service has no volume" as a bare 404 with no {error} body, the CLI would misreport it as "this backend does not support volume delete yet." Worth a quick confirmation that #189's no-volume path returns an error body. This mirrors the accepted #84 attached-flag pattern, so noting rather than blocking.

Information

• No client-side confirmation for an irreversible destroy — consistent with existing convention. --delete performs no local confirm/--force prompt, but neither do servicesRemove (src/commands/services.ts:130) nor projectDelete (src/commands/project.ts:92), which are equally irreversible. All of them delegate gating to the backend's 202 approval_required governance flow via handleApproval, which this PR correctly wires in (compute.ts:182). The --delete help text and read-line hint both warn loudly ("irreversible; download anything you need first"). Flagging only so the reliance on server-side governance is explicit.

• Security / Performance — no relevant changes. No new user input reaches SQL/shell; id is a backend-resolved value and serviceName is validated against the fetched services list (same as every sibling call site). No secrets/PII logged, no auth changes, no new dependencies. The delete path is one GET (service resolution) + one DELETE, identical in shape to the --size PUT path — no N+1, loops, or blocking work introduced.

Verification performed

• npx vitest run → 234/234 passing (matches the PR's claim), including the 18 in test/volume.test.ts.
• npx tsc -p tsconfig.json --noEmit → clean (exit 0).
• Confirmed ApiClient.rawRequest throws ApiError on any status ≥ 400 (src/api.ts:57-61), so the try/catch around the DELETE is the correct shape and the res (2xx/202) path is reachable — no dead status branches.

Verdict

approved — zero Critical findings; the two Suggestions are non-blocking. (Informational; a human still gives the explicit GitHub approval.) Note this PR is currently marked draft.

[jwfing]

Summary
This PR implements insta compute volume --delete in the expected CLI path, but it is not merge-ready because the required agent-facing CLI reference update is missing.

Requirements context
I based intent on the PR description, plus the repo’s own instructions in AGENTS.md and .claude/skills/developing-insta-cli/SKILL.md. Those documents explicitly say command/flag changes must be mirrored in skills/insta/cli-reference.md in the superproject. I did not find that file in this checkout, so the review can only verify the repo-local code and the absence of any companion doc update in this PR’s changed files. I also could not run npm test or npm run typecheck here because vitest and tsc are unavailable in the working tree.

Findings

Critical

• src/index.ts:182-188 adds a new user-facing compute volume --delete flag and updates the command contract, but the required companion update to skills/insta/cli-reference.md is not part of this change. That is an explicit repo requirement in AGENTS.md:16-17 and .claude/skills/developing-insta-cli/SKILL.md:35-37, so the feature is only half-shipped from the agent/documentation surface perspective.

Suggestion

• test/volume.test.ts:132-136 only covers the local --delete/--size conflict for computeVolume; it does not exercise the actual delete execution path (DELETE request, 202 approval passthrough, JSON mode, or success rendering). Given this command is a thin API client and the PR’s behavior depends on rawRequest/handleApproval branching in src/commands/compute.ts:176-190, an injected-client seam test would materially reduce regression risk.

Information

• (none)

Verdict
request_changes — one blocking requirement violation: the new command/flag surface is not mirrored in the required agent-facing CLI reference.

[tonychang04]

Re the round-2 CHANGES_REQUESTED: the required skills/insta/cli-reference.md mirror is not missing — that file lives in the superproject's insta-skills repo (this repo has no skills/ directory), and the update ships as InsForge/insta-skills#35, reviewed clean by both bots in round 1. The PR body now links it explicitly under "Required companion doc" so the cross-repo requirement is visible from this checkout.

On the Suggestion (injected-client seam test for the DELETE branch): the close-call logic — the older-backend 404 mapping — is now the pure exported volumeDeleteError helper with all three cases unit-pinned, per this repo's convention of testing pure seams over network orchestration (same shape as the volumeWriteLine siblings). The full wire path including the 202 approval shape is covered by the cleanroom leg in insta-e2e#16.

[jwfing]

Summary
This change cleanly adds compute-volume deletion and is consistent with the existing command/error-handling patterns in insta-cli.

Requirements context
I assessed intent primarily from the PR description, since this repo does not appear to have a fuller in-repo product spec for compute volume beyond the command registration/help text and surrounding command implementation. I also checked .claude/skills/developing-insta-cli/SKILL.md, which confirms that command/flag changes must be mirrored in the external insta-skills repo; the companion doc update referenced in the PR description is therefore the relevant place for that requirement.

Findings
Critical
(none)

Suggestion
(none)

Information
(none)

Verdict
approved.

Verification note: I inspected the implementation and tests directly. I could not execute npm test or npm run typecheck locally because this checkout does not have installed dependencies (vitest and tsc were not available).

[jwfing]

Review: feat: compute volume --delete

Summary: Adds insta compute volume <service> --delete (DELETE .../volume) with a result renderer, a pre-network --delete/--size conflict guard, and an older-backend 404 fallback; implementation is clean and consistent with sibling verbs, with one behavioral gap in the compat path.

Requirements context

No /docs/superpowers/ or /docs/specs/ directory exists in this repo (verified via ls/find — the only docs are README.md, CONTRIBUTING.md, AGENTS.md, CLAUDE.md, and the developing-insta-cli skill). Assessing against the PR description and the companion backend insta-platform#189 (feat(volume): DELETE /services/:sid/volume), whose openapi.yaml + server.ts I read to confirm the wire contract. The delete is gated server-side under governance service.remove and answers 202 approval_required when a policy requires it — the CLI's handleApproval path matches this correctly.

Findings

Critical

(none)

Suggestion

• Functionality — the older-backend 404 fallback is effectively dead against the real platform. src/commands/compute.ts:160-165 maps a route-absent 404 to the friendly "update the platform" hint only when e.message === 'HTTP 404'. That message arises (per src/api.ts:59, res.body?.error ?? 'HTTP ${status}') only when the 404 body has no error field. But insta-platform registers no setNotFoundHandler (confirmed against insta-platform@main:src/server.ts), so an older backend without this route returns Fastify's default 404 body — {"message":"Route DELETE:/... not found","error":"Not Found","statusCode":404} — which does carry error: "Not Found". So ApiError.message becomes "Not Found", never "HTTP 404", and volumeDeleteError passes it straight through. The advertised version-skew message will never fire in production; the user sees a raw Not Found instead. Consider keying the fallback on status === 404 && (message === 'Not Found' || /not found/i.test(message)) combined with the absence of a domain error, rather than the exact 'HTTP 404' sentinel.
• Software engineering — the unit test masks the above. test/volume.test.ts (the volumeDeleteError bare-404 case) hand-constructs new ApiError(404, 'HTTP 404') rather than exercising api.ts's parsing of a real Fastify 404 body, so it asserts a message shape the platform never actually produces. A test that feeds the real default-404 JSON through the same path (or a fixture derived from the backend's 404) would have surfaced the gap.

Information

• Software engineering — happy-path wiring is not integration-tested. The pure renderers (volumeDeleteLine, volumeDeleteError) and the pre-network flag-conflict throw are well covered, but the 200 → volumeDeleteLine(res.body.service?.name ?? …) and 202 → handleApproval branches in computeVolume (compute.ts:183-191) have no coverage. This is consistent with the sibling delete verbs (servicesRemove, projectDelete, branchDelete), which also leave the network path untested — noting for completeness, not asking for a change.
• Consistency (benign). Unlike the sibling deletes, which render their success line from the already-known name, this path reads res.body.service?.name first. I verified against insta-platform#189's contract that the 200 response is always a full object ({ service?, volume:null, cap, removed:true } via reply.send) with service optional-but-populated, and 4xx/5xx throw — so there is no null-deref risk here, and echoing the backend's canonical name is arguably nicer UX. No change needed.
• Security: no security-relevant changes — serviceId is resolved from the backend's own service list before being interpolated into the URL path (same as every sibling verb); no new user input reaches SQL/shell, no secrets logged, auth/authorization unchanged (destruction is gated server-side).
• Performance: no concerns — one GET /services + one DELETE, identical in shape to the existing verbs; no loops, N+1, or blocking work added.

Verdict

approved (informational — no Critical findings; the human approver still clicks approve via the separate flow). The two Suggestions concern a compat-path message that won't fire as advertised and the test that hides it; both are low-blast-radius and non-blocking. Note this PR is still marked draft.

[jwfing]

Summary
This PR implements insta compute volume --delete in a way that matches the stated behavior and existing CLI patterns, and I did not find any blocking issues.

Requirements context
I assessed the PR against: 1. the PR description, which defines the intended behavior for insta compute volume <service> --delete, the older-backend 404 compatibility mapping, the local --delete/--size conflict, and the destructive semantics; and 2. the in-repo conventions in .claude/skills/developing-insta-cli/SKILL.md, which say command/flag changes must be mirrored in skills/insta/cli-reference.md. That companion doc file is not present in this checkout, so I could not verify the doc update locally and relied on the PR description's companion insta-skills#35 note for that part.

Findings
Critical
(none)

Suggestion
(none)

Information
(none)

Software engineering: the change follows existing command patterns in src/commands/compute.ts and src/index.ts, keeps the new behavior in small pure helpers for testability, and adds focused tests for the new renderer, compatibility mapping, and local flag validation in test/volume.test.ts. Functionality: the CLI now exposes --delete, rejects --delete with --size, preserves existing 202 approval handling, and maps older route-miss 404s to the intended upgrade hint while passing domain 404s through verbatim. Security: no security-relevant changes beyond another authenticated control-plane call; no new secret handling, shell execution, or trust-boundary expansion. Performance: no meaningful performance impact beyond the same service-resolution request pattern already used by neighboring compute commands.

Verdict
approved

Local verification note: I attempted npm test -- --runInBand test/volume.test.ts and npm run typecheck, but this checkout does not have vitest or tsc available, so I could not independently rerun the suite here.

[jwfing]

Summary

Adds insta compute volume <service> --delete as a thin client over DELETE .../volume, mirroring the existing grow/PUT orchestration; the change is correct, in-scope, and well-tested, with no blocking issues.

Requirements context

No /docs/superpowers/ (or docs/specs/) directory exists in insta-cli — this repo keeps agent notes in AGENTS.md and its dev skill, not spec/plan docs. Assessed against the PR description plus the companion platform contract in insta-platform#189 (which carries the authoritative spec amendment: docs/superpowers/specs/2026-08-04-tier-resource-caps-design.md "Amendment 2026-08-08 — compute volume DELETE"). The CLI behavior matches that contract: eager destroy, service.remove governance class, any plan incl. billing-suspended, 404 = version skew vs domain error.

Findings

Critical

(none)

Suggestion

(none)

Information

• Functionality — 404 mapping is now correct, and this closes a previously-dead compat branch. src/commands/compute.ts:162-168 — GENERIC_404 = /^(HTTP 404|Not Found)$/i correctly covers both shapes an older backend can produce: a bodyless/proxy 404 (ApiError falls back to "HTTP 404") and the platform's Fastify default route-miss body {error: "Not Found"} (extracted by api.ts:59 as res.body?.error ?? 'HTTP 404' → "Not Found"). A domain 404 with a descriptive message (e.g. "this service has no volume") flows verbatim. Note this couples the CLI to the platform's default 404 body shape — if the platform ever adds a custom notFound handler that changes the error field, this mapping would silently revert to parroting the raw 404. Low risk and the code comment documents it; flagging only as a maintenance watch-point.

• Software engineering — happy-path orchestration is not unit-pinned, consistent with repo convention. src/commands/compute.ts:186-193 — the success branch (rawRequest → handleApproval → info(volumeDeleteLine(res.body.service?.name ?? …))) has no unit test; only the pure seams do (volumeDeleteLine, volumeDeleteError ×4, and the --delete+--size guard). This matches the sibling PUT/grow path (also un-pinned at the orchestration layer) and the repo's stated convention of pinning pure seams while the full wire path is exercised by the cleanroom e2e leg (insta-e2e#16). The res.body.service?.name ?? serviceName ?? id fallback is safe against the platform's Optional service field and against the 200 body shape confirmed in insta-platform#189. No change requested.

Dimension coverage

• Software engineering — tests added for all new pure seams and the flag-conflict guard (throws before any network/config access, test-pinned); import/error-handling/naming follow existing conventions (ApiError reuse, handleApproval, volumeDeleteLine mirrors volumeWriteLine). See Information notes.
• Functionality — implementation matches the platform contract (insta-platform#189): governance 202 via the shared handleApproval, verbatim domain errors, version-skew hint for generic 404s, --delete/--size mutual exclusion. Read-line hint updated only on the volume-bearing branch (nothing to delete when volumeless). No gaps found.
• Security — no security-relevant changes: thin client, no new user input reaching SQL/shell/HTTP beyond path params resolved from config/backend, no secrets logged, no auth/authz weakened (destructive gating stays server-side under service.remove), no new dependencies.
• Performance — no concerns: one services GET (id resolution, same as the existing grow path) + one DELETE; no loops, no hot-path work.

Verdict

approved (informational — human approval is a separate action). Zero Critical findings; posting as a COMMENT per the review policy.

[jwfing]

LGTM - approved.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[cubic-dev-ai]

1 issue found across 3 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/commands/compute.ts">

<violation number="1" location="src/commands/compute.ts:192">
P2: The delete success path can crash if the volume DELETE endpoint answers with an empty body (commonly 204 No Content). `ApiClient.fetch` returns `body: null` for an empty response, so `res.body.service?.name` on the non-`--json` path throws `TypeError: Cannot read properties of null` instead of printing the confirmation that the data is gone. On an irreversible destructive command that message is the one that must never be lost. The `?? serviceName ?? id` fallback already intent to degrade gracefully on an absent `.service`, but it doesn't protect against a null `res.body`. Suggest using `res.body?.service?.name ?? serviceName ?? id` so a successful empty-body delete still confirms completion.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The delete success path can crash if the volume DELETE endpoint answers with an empty body (commonly 204 No Content). ApiClient.fetch returns body: null for an empty response, so res.body.service?.name on the non---json path throws TypeError: Cannot read properties of null instead of printing the confirmation that the data is gone. On an irreversible destructive command that message is the one that must never be lost. The ?? serviceName ?? id fallback already intent to degrade gracefully on an absent .service, but it doesn't protect against a null res.body. Suggest using res.body?.service?.name ?? serviceName ?? id so a successful empty-body delete still confirms completion.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/compute.ts, line 192:

<comment>The delete success path can crash if the volume DELETE endpoint answers with an empty body (commonly 204 No Content). `ApiClient.fetch` returns `body: null` for an empty response, so `res.body.service?.name` on the non-`--json` path throws `TypeError: Cannot read properties of null` instead of printing the confirmation that the data is gone. On an irreversible destructive command that message is the one that must never be lost. The `?? serviceName ?? id` fallback already intent to degrade gracefully on an absent `.service`, but it doesn't protect against a null `res.body`. Suggest using `res.body?.service?.name ?? serviceName ?? id` so a successful empty-body delete still confirms completion.</comment>

<file context>
@@ -144,19 +144,55 @@ export function volumeWriteLine(name: string, body: { volume: { sizeGib: number;
+    catch (e) { throw volumeDeleteError(e) }
+    if (handleApproval(res)) return
+    if (opts.json) return printJson(res.body)
+    info(volumeDeleteLine(res.body.service?.name ?? serviceName ?? id))
+    return
+  }
</file context>

Suggested change

	info(volumeDeleteLine(res.body.service?.name ?? serviceName ?? id))
	info(volumeDeleteLine(res.body?.service?.name ?? serviceName ?? id))