fix: provider-honest compute suspend help copy - #162
Conversation
"RAM snapshot" was Fly-era wording and is false on the insta-compute provider, where suspend releases the microVM entirely: only the code disk and /data volume persist, and wake boots fresh in about a second. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
No issues found across 1 file
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
jwfing
left a comment
There was a problem hiding this comment.
Summary
The PR correctly updates insta compute suspend help copy to avoid claiming memory snapshots.
Requirements context
I used the PR title/description as the behavioral source because I found no linked issue and no local doc that spells out the provider suspend semantics. Local repo docs say insta --help is authoritative for command listings (README.md:193-194), and command/flag reference mirroring is required for new or renamed commands/flags (AGENTS.md:16-17, .claude/skills/developing-insta-cli/SKILL.md:36-38); this PR changes only copy for an existing command/flag surface.
Findings
Critical
(none)
Suggestion
(none)
Information
src/index.ts:230-230: Software engineering/functionality: the change is limited to the Commander description forcompute suspendand keeps the existing command registration, options, andcomputeCmd.computeSuspendaction wiring intact. Existing lifecycle tests cover runtime output rather than help-copy strings (test/compute-restart.test.ts:24-38), which is acceptable for this static one-line wording fix.src/index.ts:230-230: Security/performance: no security-relevant or performance-relevant behavior changed. There is no new untrusted input handling, logging, dependency, network path, loop, allocation, or I/O.
Verdict
Approved: no blocking findings. I did not rerun npm test or npm run typecheck because this review was constrained to read-only inspection; the PR description reports both passed.
jwfing
left a comment
There was a problem hiding this comment.
Summary
One-line help-copy fix on compute suspend that correctly kills a false "RAM snapshot" claim for the insta-compute plane — no blocking issues, but the replacement is now unconditionally true only on that plane, and the companion agent-facing doc still carries the exact stale wording this PR removes.
Requirements context
No matching spec/plan found — assessing against the PR description alone, plus the repo's own conventions. insta-cli has no /docs/superpowers/ (verified: no superpowers/, specs/ or plans/ directory anywhere in the tree). Intent taken from the PR title/body; conventions taken from AGENTS.md:14-17 and CONTRIBUTING.md:36-44.
Behavioural claims were checked against the control plane at InsForge/insta-platform@main (shallow clone), since the CLI is a thin client and suspend semantics live there.
Findings
Critical
(none)
Suggestion
Functionality — src/index.ts:230: "in-memory state does not [persist]" is stated unconditionally, but is only true on the insta-compute plane.
Fly-backed compute rows are still a live dispatch target on the platform:
insta-platform src/adapters/compute-provider.ts—COMPUTE_PROVIDERS = ['fly', 'insta-compute'], andcomputeDispatchKind()deliberately keeps routing rows toFlyAdapter(ref-shape wins over recordedkind).insta-platform src/config.ts:167-173—computeProviderKindis a create-time switch and its documented default is'fly'; "existing rows keep the kind they were born with", so legacy Fly services persist regardless of what prod sets today.insta-platform src/adapters/fly.ts:1536-1541—suspendApp()issuesPOST /apps/:app/machines/:id/suspend, i.e. Fly's real memory-snapshot suspend. For those services, in-memory state does survive.
So the PR swaps a claim that was false on one plane for a claim that is false on the other. Suggest either naming the plane or using plane-neutral wording that is honest on both, e.g. …(scales to zero; disk state persists — do not rely on in-memory state surviving). The agent-facing reference already qualifies per-plane behaviour this way for restart (insta-skills insta/cli-reference.md:51).
Functionality — src/index.ts:230: "disk/volume state persists" describes a path the platform refuses.
insta-platform src/provisioning/services.ts:1301-1303 hard-rejects a suspend on any volume-bearing service:
this service has a /data volume — machines with volumes cannot be suspended (no RAM snapshot); use stop
That guard is unconditional (not provider-gated), and this CLI's own copy already says so at src/index.ts:139 — "Volume services keep 1 machine and stop (cold wake) instead of suspend when idle". Mentioning volume persistence in the suspend description therefore points at an unreachable case and mildly contradicts the --volume copy. disk state persists alone is accurate; if you want the copy to earn its length, the more useful fact is the 400 a volume user actually hits (a service with a /data volume cannot be suspended — use \stop``).
Project convention — companion doc not updated in the same change set (CONTRIBUTING.md:40-44, AGENTS.md:16-17).
The PR body states the insta-skills suspend copy "is already provider-neutral" — true for insta/cli-reference.md:50, but the grep that backed it was repo-local and missed the sibling repo's other reference file:
InsForge/insta-skills — insta/references/operate.md:102
- `insta compute suspend [service]` — snapshot RAM for a faster resume; stays down until `start`.
That is the same false claim this PR removes, in the file coding agents read via insta setup-agent. Not a blocker for merging this repo's change, but it leaves the fix half-landed per CONTRIBUTING.md:42-44 ("Update it in the same change set"). Worth a follow-up PR in InsForge/insta-skills.
Information
- Vocabulary collision — "scales to zero". This CLI defines scale-to-zero as auto-wake behaviour (
src/index.ts:266: "off = default scale-to-zero (idle instance suspends; first connection cold-starts)"). A manual suspend setsautostart:falseso traffic will not wake it (insta-platform src/provisioning/services.ts:1283-1284). The trailing "stays down untilstart" does carry that, so the line is not wrong — but "releases the machine" would avoid overloading a term the same--helpoutput uses for the opposite wake behaviour. - The copy no longer differentiates
suspendfromstopon the new plane. Perinsta-platform src/adapters/insta-compute.ts:1629-1630, "the plane always suspends; the distinction is Fly's". With the RAM-snapshot differentiator gone,stop(src/index.ts:229) andsuspendnow read nearly identically to a user on insta-compute. That's a truthful consequence of the fix, not a defect of it — flagging only because "which one do I want?" is now unanswered by--help. - Test coverage — no gap. Nothing in
test/asserts commander description strings (no test importssrc/index.ts; nohelpInformation()usage anywhere), so shipping this without a new test matches existing convention rather than skipping TDD. Lifecycle behaviour stays covered bytest/compute-restart.test.ts. - Gates verified locally at head
876bddb(not taken on trust):npx tsc --noEmitclean;npx vitest run→ 47 test files / 672 tests passed, matching the PR body. - Security: no security-relevant changes in this PR. A static description string — no new user input reaching SQL/shell/HTTP, no secrets/PII logged or returned, no auth/authorization path touched, and
package.json/package-lock.jsonare untouched (no new or unpinned dependencies). - Performance: no performance-relevant changes in this PR. No new queries, loops, allocations, or blocking I/O; the string is built once at commander registration time.
Verdict
approved — zero Critical findings; the change is accurate for the plane it targets and the gates are green. (Informational: the GitHub green check is still a separate human approval.) The two Suggestions are worth a quick copy tweak, and the insta-skills operate.md:102 line should get a follow-up so the fix lands everywhere agents read it.
Review round 1: legacy Fly-backed services still get a true RAM-snapshot suspend, so "in-memory state does not persist" was unconditional in the wrong direction; and volume-bearing services cannot be suspended at all, so "volume state persists" pointed at an unreachable case. "Releases the machine" also avoids overloading "scales to zero", which this help output uses for auto-wake behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jwfing
left a comment
There was a problem hiding this comment.
Summary
The help copy change is narrowly scoped and accurately removes the misleading suspend semantics without touching runtime behavior.
Requirements context
I used the PR title/description as the main intent source: make insta compute suspend help provider-honest by avoiding RAM snapshot, volume persistence, and scale-to-zero wording. I found no linked issue in the provided metadata. In-repo guidance says commands are registered in src/index.ts and command/flag surface changes should be mirrored in the agent-facing reference for new or renamed command/flag changes (AGENTS.md:3-17, CONTRIBUTING.md:22-44); README treats insta --help as the authoritative command list (README.md:193-210). The checked-out commit changes only src/index.ts:230; local search found no remaining RAM snapshot or memory snapshot wording in this repo.
Findings
Critical
(none)
Suggestion
(none)
Information
(none)
Review dimensions
Software engineering: no issues; the change follows the existing Commander registration style in src/index.ts. Functionality: no issues; the final help text aligns with the stated plane-neutral suspend behavior. Security: no security-relevant changes. Performance: no performance-relevant changes.
I did not run npm run typecheck or npm test because the review instructions were read-only and tests can create local cache/artifact writes; the PR description reports both passed.
Verdict
approved
jwfing
left a comment
There was a problem hiding this comment.
Summary
The revision fixes the two problems flagged at 876bddb (the unreachable "volume state persists" clause and the "scales to zero" collision), but the replacement sentence swaps one plane's falsehood for the other's: disk state persists is false for every insta compute suspend on the insta-compute plane.
Requirements context
This repo has no /docs/superpowers/ — in fact no docs/ directory at all, and no docs/specs/. No matching spec/plan found — assessing against the PR description alone, plus the repo's own conventions in CONTRIBUTING.md:36-44, AGENTS.md:10-17 and .claude/skills/developing-insta-cli/SKILL.md. The PR body states the intent explicitly, and I hold the copy to it: "plane-neutral … the copy promises only what both planes deliver."
Note: an automated review already sits at this head (#pullrequestreview-5062589816) reporting no findings. It assessed the string against this repo only. This review adds verification against the two compute planes' current main (InsForge/insta-platform, InsForge/insta-compute) and reaches a different verdict.
Findings
Critical
Functionality — src/index.ts:230: "disk state persists" is false on the insta-compute plane, which is the only plane where the same sentence's "releases the machine" is true.
Trace of an insta compute suspend on an insta-compute-backed service, against current main of both repos:
- The platform rejects suspend outright on any volume-bearing service —
insta-platform src/provisioning/services.ts:1299-1303:throw new BadRequestError('this service has a /data volume — machines with volumes cannot be suspended (no RAM snapshot); use stop'). Not provider-gated. So this verb only ever reaches volumeless services. insta-platform src/adapters/insta-compute.ts:1625-1627→POST /services/<name>/suspend.- The plane's
SuspendService(insta-compute internal/runtime/k8s.go:825-855) scales the Deployment to 0, thenfinishSuspendflushes the img disk to object storage only underif svc.Volume != nil && meta.IsImgRef(svc.Volume.DiskRef). A volumeless service flushes nothing. - The VM's writable root filesystem is a per-pod CoW clone of the shared code disk —
internal/runtime/warm.go:1155-1160: "each claim gets a CoW clone … its own writable rootfs, like a container's overlay — deleted on release" — andk8s_warm.go:320-338releaseWarmPoddeletesrunCodeRef(pod);k8s_warm.go:450-451suspendWarm"releases the service's pool pods (deleted, never reused)". - The interface contract states exactly what survives —
internal/runtime/runtime.go:155: "SuspendService scales to 0; the volume and identity survive." The volume and identity. Not the rootfs.
So on insta-compute: nothing a running app wrote to disk survives a suspend, and the one disk that is durable across suspend (the /data volume, flushed at step 3) belongs to services the platform refuses to suspend at all. The claim is unreachable-true and reachable-false.
This matters more than the wording it replaced. RAM snapshot was a vague implication about durability; disk state persists is an explicit affirmative promise, placed in deliberate contrast with "do not rely on in-memory state surviving" — which invites exactly the reading "writes to disk survive, writes to memory don't." A user who trusts it (SQLite file, uploaded assets, a cache on the container FS) loses that data silently on the next suspend/start. That is the same class of harm this PR exists to remove, pointed at the plane it was written for.
Blocking because the PR's deliverable is the factual claim, the false clause is reachable on 100% of insta-compute suspends, and the fix is a few words.
Suggestion
Functionality — src/index.ts:230: "releases the machine" is false on the Fly plane.
insta-platform src/adapters/fly.ts:1535-1542 suspendApp posts /apps/:app/machines/:id/suspend — a real RAM snapshot; the machine is retained (fly.ts:1197-1199 documents a suspended machine that a config update moves to stopped, "because the update invalidates the RAM snapshot"). Fly-backed services are still live: insta-platform src/config.ts:169-173 documents computeProviderKind as a create-time switch — "existing rows keep the kind they were born with and are routed by it" — so legacy Fly rows persist indefinitely and computeDispatchKind keeps dispatching them.
Combined with the Critical: each clause of the new sentence is true on exactly one plane and false on the other, so the sentence is now wrong on both. The genuinely plane-invariant facts are narrower — the service stays down until start; in-memory state may not survive; only a /data volume is durable, and volume-bearing services can't suspend. Something like "takes the machine down until start; in-memory and on-disk state may not survive (only a /data volume is durable — and volume-bearing services must use stop)" is honest on both planes. On Fly it under-promises, which is the safe direction.
Functionality — the change contradicts this CLI's own copy elsewhere, and erases the suspend/stop distinction.
src/index.ts:139 still tells users "Volume services keep 1 machine and stop (cold wake) instead of suspend when idle", and src/commands/compute.ts:663 prints "suspend fast-wake and scale-out are back" after a volume delete. Both encode wake latency as the thing that makes suspend suspend. After this edit, suspend (line 230) and stop (line 229) state no user-visible difference at all — both read as "machine goes away, stays down until start" — so nothing in the help explains why you'd pick one. Wake latency is the real, plane-invariant distinction (sub-second on Fly, ~1s fresh boot per the PR body); keeping it would fix the copy without leaving the two verbs indistinguishable.
Software engineering — the agent-facing mirror still carries the exact stale claim.
InsForge/insta-skills insta/references/operate.md:102 reads "insta compute suspend [service] — snapshot RAM for a faster resume; stays down until start" (verified against that repo's current main), and insta/cli-reference.md:26 still says "suspend fast-wake … return". CONTRIBUTING.md:41-44 — "A command or flag change is only half done until it is mirrored in the agent-facing command reference … Update it in the same change set" — names cli-reference.md, whose suspend row (line 50) is already provider-neutral, so the letter of the rule is met. The spirit isn't: those files are how coding agents learn this surface, and they'd keep teaching the exact sentence this PR removes. The PR body correctly identifies this and defers it; landing the two together would avoid a window where the CLI and its own docs disagree.
Information
Software engineering — no new test is correct here, not a TDD gap. Nothing in test/ (47 files) asserts a commander description: no helpInformation, no import of src/index.ts for help output (the three commander hits in test/compute-exec.test.ts and test/resolve-service.test.ts are about argv splitting, not descriptions). A test for this one line would be a new convention, not an existing one.
Gates re-run independently in a clean clone at cf823f7: npm run typecheck exits 0 with no output; npx vitest run → 47 test files / 672 tests passed. Matches the PR body exactly. Style and one-line backticked form match the surrounding lifecycle descriptions, and insta compute --help renders the em dash and wrapping cleanly.
Security — no security-relevant changes. A single static string in the command registration: no user input, no SQL/shell/HTTP path, no auth or authorization check, no secrets or PII, no dependency changes.
Performance — no performance-relevant changes. A literal evaluated once at command-registration time; no runtime path, query, loop or allocation is touched.
Verdict
request_changes — one Critical: src/index.ts:230 promises disk state persists on the one plane where suspend actually discards the writable rootfs. Everything else here is non-blocking. The scoping, style, and gate discipline of the change are otherwise clean, and the two findings from the previous head are properly resolved.
Review round 2 Critical: "disk state persists" is false on every insta-compute suspend. The platform refuses suspend on volume-bearing services (services.ts:1305-1307), so the verb only reaches volumeless ones; there the VM's writable rootfs is a CoW clone deleted on release (warm.go:1157-1159, k8s_warm.go:450-451) and finishSuspend flushes only an img /data volume (k8s.go:843-854). The copy now promises nothing about durability, states the plane-invariant facts, and points volume-bearing services at `stop`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jwfing
left a comment
There was a problem hiding this comment.
Summary
The change removes the false RAM snapshot help claim for insta compute suspend and I found no blocking issues.
Requirements Context
I derived the intent from the PR title and description: update compute suspend help to be provider-neutral and warn users not to rely on memory or container disk state surviving. The repo docs say this CLI is a thin platform API client (AGENTS.md:3-6), insta --help is authoritative while the full command reference lives in insta-skills (README.md:193-209), and command/flag surface changes should be mirrored there (AGENTS.md:15-17, .claude/skills/developing-insta-cli/SKILL.md:36-38). I did not find an in-repo requirements document defining suspend durability semantics beyond the PR description and local command-surface docs.
Findings
Critical:
(none)
Suggestion:
(none)
Information:
- Software engineering: no issues. The one-line commander description change follows the existing
src/index.tsstyle for long lifecycle/help descriptions (src/index.ts:226-246). I did not rerunnpm run typecheckornpm testunder the read-only review constraint; the PR description reports both passed. - Functionality: no issues. The changed text is attached to
compute.command('suspend [service]')(src/index.ts:230-231), whilecomputeSuspendstill dispatches the unchanged lifecycle API call (src/commands/compute.ts:353-367), so this updates help copy without altering runtime behavior. - Security: no security-relevant changes. Only static help text changed (
src/index.ts:230-230); no input handling, auth, logging, dependencies, SQL, shell, or HTTP construction changed. - Performance: no performance-relevant changes. No runtime logic or hot path changed; the CLI still registers a static command description (
src/index.ts:230-230).
Verdict
Approved: no Critical findings. Per the InsForge bot rule, this is a non-blocking review verdict rather than a GitHub green-check approval.
jwfing
left a comment
There was a problem hiding this comment.
Summary
Round 3 @ 8af2795. The round-2 blocker is fixed — the false disk state persists promise is gone, and the new clause under-promises on both planes, which is the safe direction; one clause (until \start`) is still false on the insta-compute plane, but it is carried over verbatim in meaning from main` and is not amplified here, so it does not block.
Supersedes my CHANGES_REQUESTED at cf823f7 (#pullrequestreview-5062600424) — that Critical is resolved; I am dismissing it.
Requirements context
No matching spec/plan found — this repo has no docs/ directory at all (no docs/superpowers/, no specs/, no plans/). Assessed against the PR title/body, CONTRIBUTING.md, .claude/skills/developing-insta-cli/SKILL.md, and against current main of both compute planes (InsForge/insta-platform @ bbbfae8, InsForge/insta-compute @ cf497ff) — the same cross-repo method the PR body itself uses.
Round-2 Critical: verified resolved
Re-checked every durability claim against insta-compute @ cf497ff:
SuspendServicescales the Deployment to 0 and callsfinishSuspend(internal/runtime/k8s.go:825-830);finishSuspendflushes bytes to object storage only undersvc.Volume != nil && meta.IsImgRef(...)(internal/runtime/k8s.go:843-854) — a volumeless service, the only kind that can reachsuspend, flushes nothing.- The interface contract states exactly what survives: "SuspendService scales to 0; the volume and identity survive" (
internal/runtime/runtime.go:155). - On Fly,
suspendAppreally is a RAM snapshot (insta-platform src/adapters/fly.ts:1536-1541), and Fly rows are still live (src/config.ts:169-173, create-time switch).
do not rely on in-memory or on-disk state surviving is a negative, so it is exact on insta-compute and under-promises on Fly. Correct on both planes. This was the right fix.
The parenthetical also checks out and is genuinely provider-neutral: insta-platform src/provisioning/services.ts:1301-1302 throws before any adapter dispatch, gated on service.volume_gib != null, not on computeProviderKind.
Gates re-run in my own clone at 8af2795: npm run typecheck clean; npx vitest run → 47 files / 672 tests passed (matches the PR body); COLUMNS=100 npx tsx src/index.ts compute --help renders and wraps cleanly.
Findings
Critical
(none)
Suggestion
1. Functionality — until \start`is false on the insta-compute plane, and the PR body's verification of it cites a Fly-only mechanism ·src/index.ts:230`
The PR body says of "takes the machine down until start":
stop/suspendsetautostart:falseso the router cannot wake the app;startrestores it (insta-platform src/provisioning/services.ts:1288-1289). True on both planes.
That is a Fly-only mechanism. The insta-compute adapter's setAutostop takes two parameters — it drops the { autostart } opt entirely, and says so in its own comment:
insta-platform src/adapters/insta-compute.ts:1633— "opts.autostart has no plane equivalent (wake-on-request is constitutive); the desired-state verbs own start/stop."
And the desired-state verbs do not carry the intent either. Both stopApp and suspendApp funnel into the same body-less call (src/adapters/insta-compute.ts:1608-1627):
const r = await this.call('POST', `/services/${name}/${verb}`) // no bodyThe plane treats a body-less suspend as the historical one (insta-compute internal/api/api.go:918-923):
"The body is OPTIONAL and backward compatible: absent/empty = the historical suspend (the row wakes on traffic);
{"stop": true}= the tenant's durable stop (#138) — the executor persistsService.Stoppedwith the suspend, and the service then stays offline until an explicit wake or deploy."
So Service.Stopped stays false, and the router takes the wake branch (insta-compute internal/router/router.go:601-618):
case meta.StateSuspended:
if svc.Stopped { /* 503 "service is stopped", no wake enqueued */ }
// Hold-and-wake: enqueue one wake per service …
res := r.awaitWake(req.Context(), svc)Concretely: on an insta-compute-backed project, insta compute suspend web takes the service down, and the next HTTP request to its route — a crawler, an uptime check, a stale browser tab — wakes it back up and bills it as uptime. No start involved. A user who read the help text believes the service is off.
Why this is a Suggestion and not a blocker: main already says "stays down until start" — semantically the same claim — so this PR neither introduces nor amplifies it, and it is a strict net improvement over main (the RAM-snapshot lie and the disk-durability lie are both gone). The same falsehood also sits, in a stronger form, on the sibling line this PR does not touch (src/index.ts:229: "traffic will NOT wake it until start") and in the agent-facing reference (insta-skills insta/cli-reference.md:50). Blocking here would leave main with the worse copy.
Recommendation, in order of preference:
- The real fix is in
insta-platform, not here — have the insta-compute adapter send{"stop": true}onstopApp/suspendApp. The plane handler exists precisely for this caller (api.go:940-947names it: "the one caller this exists for is a cross-repo integration"), and once it lands, both line 229 and line 230 become true on both planes with no CLI change. Note the handler is strict: unknown fields,{"stopped": true}, and{"stop": null}are all 400s by design — send exactly{"stop": true}. - Failing that, soften the clause (e.g. "takes the machine down;
startbrings it back"), which is true on both planes without asserting traffic cannot wake it.
At minimum, please correct the clause-1 justification in the PR body before squash — it becomes the commit message, and it currently records a plane-specific mechanism as verified on both planes.
2. Software engineering — the audit leaves two sibling strings in this repo asserting the property it just removed · src/index.ts:246, src/commands/compute.ts:663
Both still promise that deleting a volume brings back "suspend fast-wake":
src/index.ts:246— "billing stops now, and suspend fast-wake + scale-out return"src/commands/compute.ts:663— "the disk and its data are gone; suspend fast-wake and scale-out are back"
"Fast-wake" is the Fly RAM-snapshot framing this PR removed from line 230 — on insta-compute there is no snapshot at all (k8s.go:825-830), so a volumeless service's wake is a cold pod start. The PR body declines a suspend-vs-stop differentiator on the grounds that "wake latency is not plane-invariant"; that reasoning applies verbatim to these two strings, which the change set left alone. (Also mirrored at insta-skills insta/cli-reference.md:26.) Not in the one-line scope, but they are the remaining instances of exactly the claim this PR exists to retire — worth a follow-up in this repo.
3. Project conventions — the docs mirror is met in letter, not in spirit · CONTRIBUTING.md:41-44
The mandated mirror target is insta-skills insta/cli-reference.md, and the PR body is right that its suspend row (line 50) makes no RAM-snapshot claim, so the rule's letter is satisfied. But insta-skills insta/references/operate.md:102 still reads:
"
insta compute suspend [service]— snapshot RAM for a faster resume; stays down untilstart."
That is the exact sentence this PR exists to retire, in the file agents read for operational guidance. The PR body discloses it as a follow-up; landing that follow-up alongside the merge (rather than after) avoids a release cycle where the CLI's help and the agent reference disagree about whether memory survives.
Information
- Test coverage — no test anywhere in
test/(47 files) asserts a commander.description()string; the four files that mentiondescription(do so incidentally. Shipping a copy change with no new test is this repo's convention, not a TDD gap..claude/skills/developing-insta-cli/SKILL.mdprescribesnpm test+npm run typecheckas the gate, and both are green at this head. - Security — no security-relevant changes in this PR. One string literal, no new input path, no dependency change, no auth surface touched.
- Performance — no performance-relevant changes in this PR. A commander description is evaluated once at program construction; no runtime path is affected.
- Style — the description's length is consistent with its neighbours (
restartatsrc/index.ts:231andvolumeat:246are both substantially longer), and it wraps correctly at 100 columns.
Verdict
approved (informational — a human still gives the GitHub approval via the approve flow). Zero Critical findings: the round-2 blocker is genuinely fixed and the new clause is safe on both planes. The three Suggestions are non-blocking — the until \start`gap is a pre-existing, unamplified claim whose real fix lives ininsta-platform`'s adapter, and the two docs items are follow-ups the PR body already anticipates.
Resolved at 8af2795: the false disk state persists promise is gone. Superseded by #162 (review)
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/index.ts">
<violation number="1" location="src/index.ts:230">
P2: The copy now errs on the opposite plane from the claim it replaces. 'do not rely on in-memory or on-disk state surviving' is accurate for insta-compute (per the reviewer note that disk state doesn't persist there), but a false warning for legacy Fly-backed services, which per the PR description still get memory-snapshot suspend with persistent disk state. Since the PR's goal is copy that promises only what both planes deliver, stating that neither in-memory nor on-disk state survives still isn't plane-neutral — it swaps the falsehood from the insta-compute plane to the Fly plane. Keep the wording to the common denominator (the machine is down until `start`) and drop the blanket state-survival claim, or scope it explicitly to insta-compute.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| compute.command('stop [service]').description('Take a compute service offline; traffic will NOT wake it until `start`') | ||
| .option('--json').option('--branch <branch>', 'branch (default: current)').action(guard((service, o) => computeCmd.computeStop(service, o))) | ||
| compute.command('suspend [service]').description('Suspend a compute service (RAM snapshot); stays down until `start`') | ||
| compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use `stop`)') |
There was a problem hiding this comment.
P2: The copy now errs on the opposite plane from the claim it replaces. 'do not rely on in-memory or on-disk state surviving' is accurate for insta-compute (per the reviewer note that disk state doesn't persist there), but a false warning for legacy Fly-backed services, which per the PR description still get memory-snapshot suspend with persistent disk state. Since the PR's goal is copy that promises only what both planes deliver, stating that neither in-memory nor on-disk state survives still isn't plane-neutral — it swaps the falsehood from the insta-compute plane to the Fly plane. Keep the wording to the common denominator (the machine is down until start) and drop the blanket state-survival claim, or scope it explicitly to insta-compute.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index.ts, line 230:
<comment>The copy now errs on the opposite plane from the claim it replaces. 'do not rely on in-memory or on-disk state surviving' is accurate for insta-compute (per the reviewer note that disk state doesn't persist there), but a false warning for legacy Fly-backed services, which per the PR description still get memory-snapshot suspend with persistent disk state. Since the PR's goal is copy that promises only what both planes deliver, stating that neither in-memory nor on-disk state survives still isn't plane-neutral — it swaps the falsehood from the insta-compute plane to the Fly plane. Keep the wording to the common denominator (the machine is down until `start`) and drop the blanket state-survival claim, or scope it explicitly to insta-compute.</comment>
<file context>
@@ -227,7 +227,7 @@ compute.command('start [service]').description('Bring a compute service online (
compute.command('stop [service]').description('Take a compute service offline; traffic will NOT wake it until `start`')
.option('--json').option('--branch <branch>', 'branch (default: current)').action(guard((service, o) => computeCmd.computeStop(service, o)))
-compute.command('suspend [service]').description('Suspend a compute service (releases the machine; disk state persists — do not rely on in-memory state surviving); stays down until `start`')
+compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use `stop`)')
.option('--json').option('--branch <branch>', 'branch (default: current)').action(guard((service, o) => computeCmd.computeSuspend(service, o)))
compute.command('restart [service]').description("Restart a compute service by re-running the image it already runs against a freshly resolved env bundle — this is how a changed secret or binding reaches a running machine (env is baked into the machine at deploy time), and how a machine that is up but wedged gets cycled (`start` no-ops on one that is already started). No new image, no new spec. The service must be running: a stopped or suspended one comes back with `insta compute start`. All plans; gated: deploy — it lands configuration the same way a deploy does, so a policy denying deploys denies this too (`start`/`stop` stay ungated, and cycle a wedged machine without one). A service whose app fails to answer on its port coming back up reports that failure, and the machines are rolled back, best-effort, to the config they were serving")
</file context>
| compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` — do not rely on in-memory or on-disk state surviving (a service with a /data volume cannot be suspended; use `stop`)') | |
| compute.command('suspend [service]').description('Suspend a compute service: takes the machine down until `start` (a service with a /data volume cannot be suspended; use `stop`)') |
insta compute suspend's help text said "RAM snapshot" — Fly-era wording that is false on the insta-compute plane, where suspend releases the microVM rather than snapshotting its memory. The description is now:Every clause is verified against current
mainof both planes rather than asserted:start" —stop/suspendsetautostart:falseso the router cannot wake the app;startrestores it (insta-platform src/provisioning/services.ts:1288-1289). True on both planes.SuspendServicescales the Deployment to 0 (insta-compute internal/runtime/k8s.go:825-830); no memory is snapshotted. On Fly,suspendAppdoes snapshot RAM (insta-platform src/adapters/fly.ts:1536-1541), so this under-promises there, which is the safe direction — and Fly rows are still live, sincecomputeProviderKindis a create-time switch (insta-platform src/config.ts:169-173).insta-compute internal/runtime/warm.go:1157-1159), andsuspendWarmreleases the pool pods, deleted and never reused (internal/runtime/k8s_warm.go:450-451).finishSuspendflushes bytes to object storage only for an img/datavolume (internal/runtime/k8s.go:843-854), and the interface contract says exactly what survives: "the volume and identity" (internal/runtime/runtime.go:155). So nothing a running app wrote to the container filesystem survives a suspend. The previous revision's "disk state persists" was an explicit affirmative promise that was false on 100% of insta-compute suspends — a user trusting it (SQLite file, uploaded assets) would lose that data silently.stop" — the platform rejects it outright, not provider-gated:insta-platform src/provisioning/services.ts:1305-1307throwsthis service has a /data volume — machines with volumes cannot be suspended (no RAM snapshot); use stop. That is why the copy makes no claim about volume durability: the one durable disk belongs to services this verb can never reach.One-line copy change, matching the surrounding lifecycle descriptions' style.
npm run typecheckclean;npm test→ 47 files / 672 tests passed;insta compute --helprenders and wraps cleanly.Declined (non-blocking, recorded rather than actioned): keeping a
suspend-vs-stopdifferentiator in the copy — wake latency is not plane-invariant (on insta-compute "the plane always suspends; the distinction is Fly's"), so any such claim would be the next false one. Landing theinsta-skillsmirror (insta/references/operate.md:102still says "snapshot RAM for a faster resume") is a follow-up PR in that repo;cli-reference.md's suspend row is already provider-neutral.🤖 Generated with Claude Code