From fa4d810c9755e5e121a31eba6ed09391664a5dcc Mon Sep 17 00:00:00 2001 From: yaowenc2 Date: Sat, 8 Aug 2026 20:46:59 -0700 Subject: [PATCH 1/3] compute volume --delete: destroy the /data volume (the way off the volume path) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETE .../volume through the same verb that attaches and grows. The backend's messages speak verbatim as always (no volume = its 404; a governance policy answers 202 approval like any destructive verb); a bare route-404 from an older backend maps to a plain "update the platform" instead of parroting HTTP 404. --delete and --size conflict locally — one changes the volume, the other destroys it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017eXAG6MxyFt5YMf22JdxJX --- src/commands/compute.ts | 44 +++++++++++++++++++++++++++++++++-------- src/index.ts | 3 ++- test/volume.test.ts | 20 ++++++++++++++++++- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/commands/compute.ts b/src/commands/compute.ts index 05f1831..57eaaf3 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -1,4 +1,4 @@ -import { ApiClient, requireProject } from '../api.js' +import { ApiClient, ApiError, requireProject } from '../api.js' import { info, printJson, handleApproval } from '../util.js' import { resolveComputeServiceId, q, parseVolumeGib } from './services.js' @@ -120,7 +120,7 @@ export function parseCpu(raw: string): number { return n } -// ---- volume (the persistent /data disk; attach any time, grow-only, never detach) ---- +// ---- volume (the persistent /data disk; attach any time, grow-only, deletable; never detach) ---- // Render the volume read. Pure, exported for tests (mirrors serviceListLine). Every plan may view; // only growth is paid — that gate is the backend's to enforce, so nothing here pre-blocks. @@ -130,7 +130,7 @@ export function volumeLines(name: string, volume: { sizeGib: number; mountPath: ] return [ `compute ${name}: volume ${volume.sizeGib}Gi at ${volume.mountPath} (plan max ${cap.volumeGib}Gi)`, - ' billing is actual data stored — the size is a cap, not a price; grow with --size (grow-only)', + ' billing is actual data stored — the size is a cap, not a price; grow with --size (grow-only), delete with --delete (destroys the data)', ] } @@ -144,19 +144,47 @@ export function volumeWriteLine(name: string, body: { volume: { sizeGib: number; return `compute ${name}: volume grown to ${body.volume.sizeGib}Gi at ${body.volume.mountPath} (plan max ${body.cap.volumeGib}Gi)` } -type VolumeOpts = LifeOpts & { size?: string } +// Render the DELETE result. Pure, exported for tests. Deleting is the only way off the volume +// path (there is no detach), so the line says what came back with it: the two constraints the +// volume imposed. +export function volumeDeleteLine(name: string): string { + return `compute ${name}: volume deleted — the disk and its data are gone; suspend fast-wake and scale-out are back` +} + +type VolumeOpts = LifeOpts & { size?: string; delete?: boolean } -// Show, attach, or grow a compute service's /data volume. No --size: a safe read (size + mount -// path + the plan cap). --size: PUT .../volume — attaches when no volume exists, grows otherwise. -// The paid/cap/machine-count gates all belong to the backend, whose 403/400 messages carry the -// upgrade hints and must reach the user verbatim (the guard prints ApiError messages as-is). +// Show, attach, grow, or delete a compute service's /data volume. No flag: a safe read (size + +// mount path + the plan cap). --size: PUT .../volume — attaches when no volume exists, grows +// otherwise. --delete: DELETE .../volume — destroys the disk and its data immediately (no detach, +// no undo; billing stops now). The paid/cap/machine-count gates all belong to the backend, whose +// 403/400 messages carry the upgrade hints and must reach the user verbatim (the guard prints +// ApiError messages as-is). export async function computeVolume(serviceName: string | undefined, opts: VolumeOpts): Promise { + if (opts.delete && opts.size) throw new Error('--delete cannot be combined with --size (one changes the volume, the other destroys it)') const api = await ApiClient.load() const p = await requireProject() const branch = opts.branch ?? p.branch const { services } = await api.request('GET', `/projects/${p.projectId}/services${q(branch)}`) const id = resolveComputeServiceId(services, serviceName) + if (opts.delete) { + let res + try { res = await api.rawRequest('DELETE', `/projects/${p.projectId}/services/${id}/volume`) } + catch (e) { + // An older backend has no DELETE route and answers a bare 404 (no error body) — tell the + // user what is missing instead of parroting "HTTP 404". A backend that HAS the route says + // "this service has no volume" (or names the real problem), and that message flows as-is. + if (e instanceof ApiError && e.status === 404 && /^HTTP 404$/.test(e.message)) { + throw new Error('this backend does not support volume delete yet — update the platform, or delete the service to remove its volume') + } + throw e + } + if (handleApproval(res)) return + if (opts.json) return printJson(res.body) + info(volumeDeleteLine(res.body.service?.name ?? serviceName ?? id)) + return + } + if (!opts.size) { const r = await api.request('GET', `/projects/${p.projectId}/services/${id}/volume`) if (opts.json) return printJson(r) diff --git a/src/index.ts b/src/index.ts index 3207d93..ed8488c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -182,8 +182,9 @@ compute.command('limits [service]').description("Show or set a compute service's .option('--json').option('--branch ', 'branch (default: current)').action(guard((service, o) => computeCmd.computeLimits(service, o))) compute.command('always-on [service]').description('Set a compute service always-on (mode: on|off). on = machines never scale to zero; off = default scale-to-zero. All plans; billing is actual usage either way') .option('--json').option('--branch ', 'branch (default: current)').action(guard((mode, service, o) => computeCmd.computeAlwaysOn(mode, service, o))) -compute.command('volume [service]').description("Show, attach, or grow a compute service's persistent /data volume. No --size: print size, mount path, and the plan cap (any plan). --size on a volumeless service ATTACHES one (any plan at the default 1Gi; larger is paid and plan-capped; the disk mounts at /data on the next deploy); on a volume-bearing one it grows (paid plans; grow-only — a provisioned disk cannot shrink). Billing is actual data stored — the size is a cap, not a price") +compute.command('volume [service]').description("Show, attach, grow, or delete a compute service's persistent /data volume. No flag: print size, mount path, and the plan cap (any plan). --size on a volumeless service ATTACHES one (any plan at the default 1Gi; larger is paid and plan-capped; the disk mounts at /data on the next deploy); on a volume-bearing one it grows (paid plans; grow-only — a provisioned disk cannot shrink). --delete DESTROYS the disk and ALL its data immediately (no detach, no undo; billing stops now, and suspend fast-wake + scale-out return). Billing is actual data stored — the size is a cap, not a price") .option('--size ', 'new size in whole Gi, e.g. 10 (must be ≥ the current size)') + .option('--delete', 'destroy the volume and ALL its data (irreversible; download anything you need first)') .option('--json').option('--branch ', 'branch (default: current)').action(guard((service, o) => computeCmd.computeVolume(service, o))) // ---- db (postgres service controls) ---- diff --git a/test/volume.test.ts b/test/volume.test.ts index 5cbb813..89ec081 100644 --- a/test/volume.test.ts +++ b/test/volume.test.ts @@ -7,7 +7,7 @@ // deprecated storage* aliases the platform drops next release. import { describe, it, expect } from 'vitest' import { parseVolumeGib, servicesAddRequestBody, servicesAdd, serviceListLine } from '../src/commands/services.js' -import { volumeLines, volumeWriteLine } from '../src/commands/compute.js' +import { volumeLines, volumeWriteLine, volumeDeleteLine, computeVolume } from '../src/commands/compute.js' import { dbVolumeLines } from '../src/commands/db.js' describe('parseVolumeGib', () => { @@ -67,6 +67,8 @@ describe('volumeLines (compute read display)', () => { const lines = volumeLines('api', { sizeGib: 10, mountPath: '/data' }, { volumeGib: 50 }) expect(lines[0]).toBe('compute api: volume 10Gi at /data (plan max 50Gi)') expect(lines[1]).toMatch(/cap, not a price/) + // The read is where a user learns the way OFF the volume path exists — and what it costs. + expect(lines[1]).toMatch(/--delete \(destroys the data\)/) }) it('points a volumeless service at the attach verb (this command with --size)', () => { const lines = volumeLines('api', null, { volumeGib: 50 }) @@ -92,6 +94,22 @@ describe('volumeWriteLine (compute PUT result display)', () => { }) }) +describe('volumeDeleteLine (compute DELETE result display)', () => { + // The delete line's job is closure: the data is gone (no detach existed, no undo exists), and + // the two constraints the volume imposed — cold stop/start wake and machineCount 1 — left with + // it. No cap/size: there is nothing left to size. + it('says the disk and data are gone and both constraints are back', () => { + const line = volumeDeleteLine('api') + expect(line).toBe('compute api: volume deleted — the disk and its data are gone; suspend fast-wake and scale-out are back') + }) +}) + +describe('computeVolume --delete validation (throws before any network/config access)', () => { + it('rejects --delete combined with --size — one changes the volume, the other destroys it', async () => { + await expect(computeVolume('api', { delete: true, size: '10' })).rejects.toThrow(/--delete cannot be combined with --size/) + }) +}) + describe('dbVolumeLines (postgres read display)', () => { it('reads the canonical volumeGib + cap, and shows region when the instance reports one', () => { const lines = dbVolumeLines('default', { volumeGib: 10, volumeSize: '10Gi', cap: { volumeGib: 50 }, region: 'us-east' }) From 3397bfcd9fb26e62650bc352b40699ab835bf3c2 Mon Sep 17 00:00:00 2001 From: yaowenc2 Date: Sat, 8 Aug 2026 21:15:14 -0700 Subject: [PATCH 2/3] Review round 1: extract + pin the older-backend 404 mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers flagged the bare-404-vs-404-with-body branch as the close call worth testing. It is now a pure exported helper (volumeDeleteError) with the three cases pinned: bare route-404 → version-skew hint; 404 with a body message → verbatim (that backend has the route — confirmed live against the platform branch: a volumeless service answers {"error":"this service has no volume"}); everything else untouched. 237/237. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017eXAG6MxyFt5YMf22JdxJX --- src/commands/compute.ts | 23 ++++++++++++++--------- test/volume.test.ts | 23 ++++++++++++++++++++++- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/commands/compute.ts b/src/commands/compute.ts index 57eaaf3..f877498 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -151,6 +151,19 @@ export function volumeDeleteLine(name: string): string { return `compute ${name}: volume deleted — the disk and its data are gone; suspend fast-wake and scale-out are back` } +// Map a DELETE .../volume failure. Pure, exported for tests (r2d2 review: this is the close-call +// branch worth pinning). An older backend has no DELETE route and answers a BARE 404 — no error +// body, so ApiError's message is the literal "HTTP 404" fallback — and parroting that would send +// the user hunting a bug that is really a version skew. A backend that HAS the route always names +// the real problem in an error body ("this service has no volume", …), and that message must flow +// verbatim, 404 or not. +export function volumeDeleteError(e: unknown): unknown { + if (e instanceof ApiError && e.status === 404 && e.message === 'HTTP 404') { + return new Error('this backend does not support volume delete yet — update the platform, or delete the service to remove its volume') + } + return e +} + type VolumeOpts = LifeOpts & { size?: string; delete?: boolean } // Show, attach, grow, or delete a compute service's /data volume. No flag: a safe read (size + @@ -170,15 +183,7 @@ export async function computeVolume(serviceName: string | undefined, opts: Volum if (opts.delete) { let res try { res = await api.rawRequest('DELETE', `/projects/${p.projectId}/services/${id}/volume`) } - catch (e) { - // An older backend has no DELETE route and answers a bare 404 (no error body) — tell the - // user what is missing instead of parroting "HTTP 404". A backend that HAS the route says - // "this service has no volume" (or names the real problem), and that message flows as-is. - if (e instanceof ApiError && e.status === 404 && /^HTTP 404$/.test(e.message)) { - throw new Error('this backend does not support volume delete yet — update the platform, or delete the service to remove its volume') - } - throw e - } + catch (e) { throw volumeDeleteError(e) } if (handleApproval(res)) return if (opts.json) return printJson(res.body) info(volumeDeleteLine(res.body.service?.name ?? serviceName ?? id)) diff --git a/test/volume.test.ts b/test/volume.test.ts index 89ec081..6112304 100644 --- a/test/volume.test.ts +++ b/test/volume.test.ts @@ -7,7 +7,8 @@ // deprecated storage* aliases the platform drops next release. import { describe, it, expect } from 'vitest' import { parseVolumeGib, servicesAddRequestBody, servicesAdd, serviceListLine } from '../src/commands/services.js' -import { volumeLines, volumeWriteLine, volumeDeleteLine, computeVolume } from '../src/commands/compute.js' +import { volumeLines, volumeWriteLine, volumeDeleteLine, volumeDeleteError, computeVolume } from '../src/commands/compute.js' +import { ApiError } from '../src/api.js' import { dbVolumeLines } from '../src/commands/db.js' describe('parseVolumeGib', () => { @@ -104,6 +105,26 @@ describe('volumeDeleteLine (compute DELETE result display)', () => { }) }) +describe('volumeDeleteError (older-backend 404 mapping)', () => { + // The close-call branch: a bare route-404 (no error body → ApiError falls back to the literal + // "HTTP 404") means the BACKEND is old; any 404 that carries a message came from a backend that + // HAS the route and is naming the real problem — verified live against feat/volume-remove + // (dev:fake): a volumeless service answers `{"error":"this service has no volume"}`. + it('maps a bare route-404 to the version-skew hint', () => { + const out = volumeDeleteError(new ApiError(404, 'HTTP 404')) as Error + expect(out.message).toMatch(/does not support volume delete yet/) + }) + it('passes a 404 WITH a body message through verbatim — that backend has the route', () => { + const e = new ApiError(404, 'this service has no volume') + expect(volumeDeleteError(e)).toBe(e) + }) + it('passes every non-404 through untouched (403 governance, 502 provider, plain errors)', () => { + for (const e of [new ApiError(403, 'approval required'), new ApiError(502, 'provider failed'), new Error('boom')]) { + expect(volumeDeleteError(e)).toBe(e) + } + }) +}) + describe('computeVolume --delete validation (throws before any network/config access)', () => { it('rejects --delete combined with --size — one changes the volume, the other destroys it', async () => { await expect(computeVolume('api', { delete: true, size: '10' })).rejects.toThrow(/--delete cannot be combined with --size/) From ac768cd58b75a2008cc15bf5873f1d8aa0675857 Mon Sep 17 00:00:00 2001 From: yaowenc2 Date: Sat, 8 Aug 2026 21:26:35 -0700 Subject: [PATCH 3/3] Review round 2: the compat 404 hint now fires against a REAL old platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit r2d2 caught that the fallback was dead code in production: an older platform is Fastify with no custom notFound handler, so its route-miss 404 carries error:"Not Found" — which sailed past the exact "HTTP 404" sentinel and reached the user raw. The mapping now treats both generic route-miss shapes ("HTTP 404" bodyless, "Not Found" Fastify default) as version skew, while domain 404s ("this service has no volume") still flow verbatim. New test derives its fixture from the literal Fastify default body through the same extraction rawRequest applies, so shape drift on either side breaks it. 238/238. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017eXAG6MxyFt5YMf22JdxJX --- src/commands/compute.ts | 17 ++++++++++------- test/volume.test.ts | 8 ++++++++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/commands/compute.ts b/src/commands/compute.ts index f877498..91cb4f0 100644 --- a/src/commands/compute.ts +++ b/src/commands/compute.ts @@ -151,14 +151,17 @@ export function volumeDeleteLine(name: string): string { return `compute ${name}: volume deleted — the disk and its data are gone; suspend fast-wake and scale-out are back` } -// Map a DELETE .../volume failure. Pure, exported for tests (r2d2 review: this is the close-call -// branch worth pinning). An older backend has no DELETE route and answers a BARE 404 — no error -// body, so ApiError's message is the literal "HTTP 404" fallback — and parroting that would send -// the user hunting a bug that is really a version skew. A backend that HAS the route always names -// the real problem in an error body ("this service has no volume", …), and that message must flow -// verbatim, 404 or not. +// Map a DELETE .../volume failure. Pure, exported for tests (r2d2 review rounds 1+2: this is the +// close-call branch worth pinning). An older backend has no DELETE route, and what its 404 looks +// like depends on who answered: the real platform (Fastify, no custom notFound handler) sends its +// default body {"message":"Route DELETE:/… not found","error":"Not Found"} → ApiError message +// "Not Found"; a proxy or bodyless 404 leaves ApiError's own "HTTP 404" fallback. BOTH are the +// generic route-miss shape and mean version skew, not a bug — parroting them would send the user +// hunting the wrong thing. A backend that HAS the route names the real problem in a DOMAIN +// message ("this service has no volume", …), which must flow verbatim, 404 or not. +const GENERIC_404 = /^(HTTP 404|Not Found)$/i export function volumeDeleteError(e: unknown): unknown { - if (e instanceof ApiError && e.status === 404 && e.message === 'HTTP 404') { + if (e instanceof ApiError && e.status === 404 && GENERIC_404.test(e.message.trim())) { return new Error('this backend does not support volume delete yet — update the platform, or delete the service to remove its volume') } return e diff --git a/test/volume.test.ts b/test/volume.test.ts index 6112304..ce65db1 100644 --- a/test/volume.test.ts +++ b/test/volume.test.ts @@ -114,6 +114,14 @@ describe('volumeDeleteError (older-backend 404 mapping)', () => { const out = volumeDeleteError(new ApiError(404, 'HTTP 404')) as Error expect(out.message).toMatch(/does not support volume delete yet/) }) + it('maps the REAL older-platform 404: the Fastify default body parses to "Not Found" (r2d2 round 2)', () => { + // The exact body an older platform (Fastify, no custom notFound handler) sends for a missing + // route, pushed through the same extraction rawRequest applies (`body?.error ?? "HTTP 404"`) — + // the fixture derivation r2d2 asked for, so this test breaks if either side's shape drifts. + const fastifyDefault404 = { message: 'Route DELETE:/projects/p/services/s/volume not found', error: 'Not Found', statusCode: 404 } + const e = new ApiError(404, (fastifyDefault404 as { error?: string }).error ?? 'HTTP 404') + expect((volumeDeleteError(e) as Error).message).toMatch(/does not support volume delete yet/) + }) it('passes a 404 WITH a body message through verbatim — that backend has the route', () => { const e = new ApiError(404, 'this service has no volume') expect(volumeDeleteError(e)).toBe(e)