feat(compute): add mount path and startup command controls - #264
Conversation
|
src/index.ts:311 的 compute volume 命令描述仍然只按 attach 场景讲 --mount-path（"the disk mounts at --mount-path (default /data) on the next deploy"），没有提到现在它可以单独用来改已有卷的路径、也没提 pending / 无变化时是 no-op 读回。--mount-path 的 option help 已经更新了，但命令级描述才是 insta compute volume --help 里最显眼的那段。 另外：platform#504 新增了 startCommand，CLI 这边没有任何对应的 flag。skills#120 因此只能把 agent 指向 Console —— 而 Console 那边对已存在的服务同样点不到（见 console#502 的 comment）。结果是 startCommand 对已有服务只有裸 API 一条路，这正是新增的 e2e probe 所做的。 |
|
Addressed in Addressed both points:
Validation: typecheck/build passed; full CLI suite passed 1,711 tests. All 13 focused volume/startup-command tests passed after the final clear-command correction; the companion platform API reset test also passed. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Removes the --mount-path requires --size guard so an existing compute volume's path can be restaged, renders the new pending / changed fields from the volume API, and adds a new insta compute start-command verb — but dropping that guard also makes --mount-path alone implicitly attach a brand-new volume at the org plan cap on a volumeless service, and all three new output branches are unbound by any test.
Requirements context
No matching spec/plan found — instacloud-cli has no docs/, docs/superpowers/ or plans/ directory at bc29bcd, so intent is taken from the PR title/body, the --help strings this PR writes, and the two companion PRs it names. I read the companion platform PR (InsForge/instacloud-platform#504) diff to verify the wire contract the CLI now assumes; the parts the CLI depends on check out: volume.{pending,appliedMountPath} and top-level changed are real response fields (src/provisioning/services.ts volumeView / setVolume), PATCH /services/:id accepts startCommand with maxLength: 8192, and startCommand: '' is mapped to NULL (startCommand.trim() || null, asserted by that PR's own expect(clear.json().service.start_command).toBeNull()), so --clear sending '' is correct.
Gates run in a fresh clone at bc29bcd: npm ci + npx vitest run → 84 files, 1710 passed | 1 skipped, matching the body's claim.
Findings
Critical
1. Functionality / scope — --mount-path with no --size silently ATTACHES a volume on a volumeless service, at the plan cap — src/commands/compute.ts:728-754
The deleted guard (--mount-path requires --size when attaching a volume) was the only thing stopping --mount-path from provisioning a disk. The new code path is:
if (!opts.size && opts.mountPath === undefined) { /* read */ }
const sizeGib = opts.size === undefined ? undefined : parseVolumeGib(opts.size)
await api.rawRequest('PUT', `.../volume`, { sizeGib, ...(opts.mountPath !== undefined ? { mountPath: opts.mountPath } : {}) })sizeGib: undefined is dropped by JSON.stringify, so insta compute volume web --mount-path /app/storage sends PUT {mountPath: '/app/storage'}. On the platform, ServicesService.setVolume (src/provisioning/services.ts, already on platform main, not changed by #504) takes the attach branch whenever service.volume_gib == null, and:
const attachGib = sizeGib ?? cap.volumeGib // free 10Gi, paid 50Gi on shipped config
...
return { ..., changed: true, attached: true }So on a compute service with no volume, a path-only invocation provisions a disk at the org's full plan cap, and volumeWriteLine cheerfully prints volume 50Gi attached — mounts at /app/storage on the next deploy. That is outside this PR's own stated scope ("path-only updates to existing compute volumes", and index.ts:316-317 "--mount-path alone stages an existing volume path change"), and it is not a cheap accident: the service permanently loses suspend fast-wake and scale-out (this repo's own help text at index.ts:316 says so), the disk is billable, and removing it means --delete, which destroys data.
Evidence that this is an oversight rather than intent: platform #504 added exactly this guard to the other route it touched —
if (patch.volumeMountPath !== undefined && patch.volumeGib === undefined && service.volume_gib == null)
throw new BadRequestError('volumeMountPath requires an existing volume or volumeGib')— on applySettings (PATCH /services/:id), but PUT /services/:id/volume, the route this CLI change actually calls, has no equivalent check.
The PR's own new test encodes the bug rather than catching it (test/volume-mount-path.test.ts:38-41 asserts exactly PUT {mountPath: '/cache', sizeGib: undefined} with no volume-existence precondition), and the old test that would have flagged it was deleted (test/volume.test.ts:188).
Fix options, either is fine: (a) when only --mount-path is given, GET .../volume first and fail with a clear message if volume is null ("no volume attached — pass --size <gi> to attach one"); or (b) land the missing volume_gib == null guard on the platform PUT /volume route in #504 and let the 400 flow through. (a) alone keeps the CLI honest without another round-trip on the platform side.
Suggestion
2. Functionality — the pending branch shadows the grow/no-op lines, because pending means "configured ≠ applied", not "the user just changed the path" — src/commands/compute.ts:670, 679-680
Platform volumeView computes pending: mountPath !== service.volume_applied_mount_path, and volume_applied_mount_path is only written at deploy time (update services set volume_applied_mount_path = $2, volume_applied_id = $3). So any volume that has not been deployed yet reports pending: true with appliedMountPath: null, even when the user never touched the path. Three reachable consequences:
insta compute volume web --size 20on a volume attached but not yet deployed →attached: false,pending: true→ line 679 fires and printsmount path (not deployed) → /data pending — deploy to apply …. The user asked to grow a paid disk and is never toldvolume grown to 20Gi; they're instead told about a mount-path change they didn't make.insta compute volume web --size 20 --mount-path /app/storageon a deployed volume → platform returnsattached: false, pending: true, changed: true(changed: changed || pathChanged), so line 679 again wins and the size grow is never confirmed. This arm is reachable at any time, not just pre-deploy.- The
changed === falseno-op line (680) is unreachable on any not-yet-deployed volume, so the "normalized no-op updates" distinction the PR body claims doesn't hold there. - Same for the read:
volumeLines(670) printspending: (not deployed) → /data; deploy to apply (restarts the service)on every freshly attached volume, which is noise on the most common read.
Suggest keying the pending line on an actual staged edit (e.g. body.volume.pending && body.volume.appliedMountPath != null, or only when the request carried --mount-path), and appending rather than replacing the grow/attach sentence so a size change is always confirmed.
3. Software engineering — none of the three new render branches is covered; a negative control proves it — src/commands/compute.ts:670, 679, 680
grep -rn "pending\|appliedMountPath\|changed" test/volume*.ts returns nothing, and test/volume.test.ts:102-113 (volumeWriteLine) still only exercises attach/grow. I deleted all three new lines in a copy of the tree and ran the whole suite: 84 files, 1710 passed | 1 skipped — identical to the unmodified tree. The headline user-visible behavior of this PR ("output distinguishes pending/applied paths and normalized no-op updates") is entirely unbound. volumeLines/volumeWriteLine are pure and already exported for exactly this purpose, so these are cheap table tests: {pending: true, appliedMountPath: '/data'}, {pending: true, appliedMountPath: null}, {changed: false}, and the attached: true + pending: true first-attach case that must not take the new branch. Those tests would also have surfaced finding 2.
4. Software engineering — the --size '' behaviour change is silent and now produces a worse message — src/commands/compute.ts:753
The deleted it.each([undefined, '']) case (test/volume-mount-path.test.ts:38) covered --size ''. With --mount-path /cache --size '', opts.size is '' (not undefined), so parseVolumeGib('') throws invalid volume size: (whole Gi — try 1 or 10) — an empty interpolation, which reads as a truncated error. Worth either a guard (!opts.size → treat as unset) or a test pinning whichever behaviour is intended.
5. Software engineering — start-command read display is unbound and the JSON shape is ad hoc — src/commands/compute.ts:2067-2070
test/volume-mount-path.test.ts:61-63 only asserts that rawRequest was not called; nothing asserts what the read prints. The read reaches for start_command off the list response via an inline cast (svc as { start_command?: string | null }); if that field ever isn't on the list payload, the command silently prints (image default) for a service that has a command set — a wrong read with no test to catch it. (#504 does add start_command to S.Service, so it works today.) Separately, printJson({ service: svc }) on the read dumps the raw list row, whereas every sibling read in this file prints the API response verbatim (printJson(r)); a caller parsing --json gets a shape unique to this one command.
Information
- Scope — the PR title and branch name are about mount paths, but roughly half the diff is a new top-level verb (
compute start-command). It's disclosed in the body, and the two are related in the "stage everything, deploy once" story, but a reviewer looking at "path-only volume updates" would not expect a new command. Its tests also live intest/volume-mount-path.test.ts, which will be a surprising place to find them later. - Security — no new secret handling, no auth weakening, no new dependencies.
--setis user text that the platform runs as['sh', '-c', svc.start_command]inside the caller's own container; that's the feature, not an injection — and the platform's agent-governance table in #504 correctly classifiesstartCommandas requiring thedeployaction, which the CLI routes through the existinghandleApproval. No client-side validation of--mount-pathis added, but the repo's convention (services.tsparseVolumeGibaside) is to let backend 400s flow verbatim, so that's consistent. - Performance — no concerns.
computeStartCommanddoes one listGETplus onePATCH, matching every sibling verb in this file; no new loops, no per-request allocation, no DB work in the CLI. - Convention —
info((svc as …).start_command || '(image default)')(compute.ts:2069) prints a bare value, while every other read incompute.tsprefixes${type} ${name}:. Minor, but it makes the line hard to read in a scrollback. index.ts:138correctly leaves theservice add --mount-pathguard intact (services.ts:140still requires--volume); only thecompute volumeguard was dropped. Worth noting because finding 1 is exactly the gap between those two.
Verdict
request_changes — finding 1 (path-only --mount-path implicitly provisions a plan-cap volume on a volumeless service) should be closed before merge. Findings 2–5 are non-blocking.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
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="test/volume-mount-path.test.ts">
<violation number="1" location="test/volume-mount-path.test.ts:40">
P3: This commit is titled 'report volume changes accurately', but the added tests still exercise none of the new path-only write rendering: `volumeWriteLine` branches for `pending`/`appliedMountPath` and `changed === false`/`attached` (compute.ts 678-690) and the `volumeLines` pending line (compute.ts 664-665) get no assertion. The modified path-only test checks only the PUT request body against the default `rawRequest` mock, whose fixed `{ attached: true, volume: {sizeGib:1, mountPath:'/app/storage'} }` shape never produces a pending or no-op outcome, so the branches the PR claims are unverified. Add assertions on `volumeWriteLine`/read output for pending-path and unchanged cases.</violation>
<violation number="2" location="test/volume-mount-path.test.ts:91">
P3: The `info` and `printJson` mocks created in the `vi.mock('../src/util.js', ...)` factory are never reset: `beforeEach` only resets `fake.request`/`fake.rawRequest`/`fake.load`, and vitest.config has no `clearMocks`. `toHaveBeenCalledWith` therefore inspects the full accumulated history for the whole file, so these assertions can silently pass stale once any earlier test emits the same arguments. Reset both mocks in `beforeEach` alongside the `fake.*` resets.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| await computeStartCommand('web', {}) | ||
| expect(info).toHaveBeenCalledWith('compute web: startup command exec app') | ||
| await computeStartCommand('web', { json: true }) | ||
| expect(printJson).toHaveBeenCalledWith({ service }) |
There was a problem hiding this comment.
P3: The info and printJson mocks created in the vi.mock('../src/util.js', ...) factory are never reset: beforeEach only resets fake.request/fake.rawRequest/fake.load, and vitest.config has no clearMocks. toHaveBeenCalledWith therefore inspects the full accumulated history for the whole file, so these assertions can silently pass stale once any earlier test emits the same arguments. Reset both mocks in beforeEach alongside the fake.* resets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/volume-mount-path.test.ts, line 91:
<comment>The `info` and `printJson` mocks created in the `vi.mock('../src/util.js', ...)` factory are never reset: `beforeEach` only resets `fake.request`/`fake.rawRequest`/`fake.load`, and vitest.config has no `clearMocks`. `toHaveBeenCalledWith` therefore inspects the full accumulated history for the whole file, so these assertions can silently pass stale once any earlier test emits the same arguments. Reset both mocks in `beforeEach` alongside the `fake.*` resets.</comment>
<file context>
@@ -69,3 +71,22 @@ describe('startup command staging', () => {
+ await computeStartCommand('web', {})
+ expect(info).toHaveBeenCalledWith('compute web: startup command exec app')
+ await computeStartCommand('web', { json: true })
+ expect(printJson).toHaveBeenCalledWith({ service })
+})
</file context>
| expect(fake.request).not.toHaveBeenCalled() | ||
| expect(fake.rawRequest).not.toHaveBeenCalled() | ||
| it('sends a path-only edit without an implicit resize', async () => { | ||
| fake.request.mockResolvedValueOnce({ services: [{ id: 's1', type: 'compute', name: 'web' }] }).mockResolvedValueOnce({ volume: { sizeGib: 1, mountPath: '/data' } }) |
There was a problem hiding this comment.
P3: This commit is titled 'report volume changes accurately', but the added tests still exercise none of the new path-only write rendering: volumeWriteLine branches for pending/appliedMountPath and changed === false/attached (compute.ts 678-690) and the volumeLines pending line (compute.ts 664-665) get no assertion. The modified path-only test checks only the PUT request body against the default rawRequest mock, whose fixed { attached: true, volume: {sizeGib:1, mountPath:'/app/storage'} } shape never produces a pending or no-op outcome, so the branches the PR claims are unverified. Add assertions on volumeWriteLine/read output for pending-path and unchanged cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/volume-mount-path.test.ts, line 40:
<comment>This commit is titled 'report volume changes accurately', but the added tests still exercise none of the new path-only write rendering: `volumeWriteLine` branches for `pending`/`appliedMountPath` and `changed === false`/`attached` (compute.ts 678-690) and the `volumeLines` pending line (compute.ts 664-665) get no assertion. The modified path-only test checks only the PUT request body against the default `rawRequest` mock, whose fixed `{ attached: true, volume: {sizeGib:1, mountPath:'/app/storage'} }` shape never produces a pending or no-op outcome, so the branches the PR claims are unverified. Add assertions on `volumeWriteLine`/read output for pending-path and unchanged cases.</comment>
<file context>
@@ -36,6 +37,7 @@ describe('volume mount path requests', () => {
describe('mount path validation and list display', () => {
it('sends a path-only edit without an implicit resize', async () => {
+ fake.request.mockResolvedValueOnce({ services: [{ id: 's1', type: 'compute', name: 'web' }] }).mockResolvedValueOnce({ volume: { sizeGib: 1, mountPath: '/data' } })
await computeVolume('web', { mountPath: '/cache' })
expect(fake.rawRequest).toHaveBeenCalledWith('PUT', '/projects/p1/services/s1/volume', { mountPath: '/cache', sizeGib: undefined })
</file context>
|
Updated in 638c055. Addressed the review findings:
The platform's current grow response reports changed:true; the display now preserves that result rather than allowing pending-path copy to hide it. Validation: typecheck, build, and full suite: 84 files / 1,717 tests passed. |
v01dstar
changed the title
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Round 2 @638c055: the Critical I raised in round 1 is fixed on both sides — the CLI now pre-flights a path-only edit and refuses a volumeless service, and platform #504 has since added the matching guard to the PUT /services/:id/volume route it was missing. No blocking findings this round; four Suggestions and three Information notes below.
Requirements context
instacloud-cli still has no docs/ or plans/ directory, so no spec/plan document under /docs/superpowers/ exists for this repo. The intent source is the tracking issue InsForge/instacloud#153 ("Phase 2 — Change Existing Compute Mount Paths"), whose CLI and Console section I graded against:
| Requirement (issue #153) | Status |
|---|---|
insta compute volume web --mount-path /app/storage |
✅ src/commands/compute.ts:755-760 |
| Display old path, new path, pending status, restart notice | ✅ compute.ts:670, compute.ts:679 |
| Explain application configuration is not updated automatically | ✅ both render paths carry the sentence |
| Treat an unchanged path as a no-op | ✅ compute.ts:681 ("volume unchanged") |
| Env vars and startup commands change in the same deployment |
Companion state at time of review: instacloud-platform#504 is still open (head bf43024, not merged).
Round-1 Critical — verified closed
The r1 block was: --mount-path alone dropped sizeGib via JSON.stringify, and platform setVolumeSize took the volumeless-attach branch with attachGib = sizeGib ?? cap.volumeGib, silently provisioning a disk at the org plan cap (10/50Gi) and permanently costing the service suspend fast-wake + scale-out.
Both halves are now closed, and I confirmed each:
- Client —
compute.ts:756-758GETs the volume first and throwsno volume attached; attach one with…before any write. Test-pinned: deleting the guard turns the volume suites red (1 failed / 44 passed). - Platform — #504
services.tssetVolumeSizenow opens withif (mountPath !== undefined && sizeGib === undefined && service.volume_gib == null) throw new BadRequestError(...). In r1 that guard existed only onapplySettings; the sibling route is now covered, so non-CLI callers are protected too, as the PR body claims.
Negative controls run on the whole new surface (npx vitest run on the volume suites, each arm reverted separately):
| Arm | Result |
|---|---|
| A — delete client path-only guard | 🔴 1 failed |
B — delete empty---size guard |
🔴 1 failed |
C — delete volumeLines pending line |
🔴 1 failed |
D — neuter volumeWriteLine pending suffix |
🔴 1 failed |
E — delete changed === false branch |
🔴 1 failed |
F — force sizeRequested true |
🔴 1 failed |
G — drop appliedMountPath != null in volumeWriteLine |
🟢 45 passed |
This is a marked improvement on round 1, where deleting all three render lines left the suite byte-identically green. Full suite at head: 84 files, 1716 passed / 1 skipped, matching the author's stated validation.
Findings
Critical
(none)
Suggestion
1. Software engineering — the "first attachment is not a remount" guard is unbound (src/commands/compute.ts:679).
Arm G above: removing && body.volume.appliedMountPath != null leaves all 45 volume tests green, yet that conjunct is the only thing preventing a literal null from being printed. Measured at head with the conjunct removed:
compute web: volume 1Gi at /new (plan max 50Gi); mount path null → /new pending — deploy or restart to apply. …
compute web: volume unchanged: 1Gi at /new; mount path null → /new pending — deploy or restart to apply. …
This is reachable, not theoretical: attach a volume, don't deploy, then change its path — which is exactly the stage-then-deploy workflow this PR advertises. #504's volumeView returns appliedMountPath: service.volume_applied_mount_path ?? null with pending: mountPath !== service.volume_applied_mount_path, so a never-deployed volume yields {appliedMountPath: null, pending: true}, and both the changed:true (path-only and grow+path) and changed:false returns append the suffix.
The test that appears to cover this, test/volume.test.ts:199-204 ("does not call a first attachment a remount"), cannot bind it: line 201 goes through the attached branch, which never appends pending at all, and line 203's toContain('unchanged: 1Gi') still passes with the bogus suffix appended. Changing line 203 to expect(...).not.toContain('pending') closes it.
2. Functionality — the immediate-redeploy warning is on the staging commands, not the firing one.
src/index.ts:311 and :317 both warn that "CLI secrets writes redeploy immediately", but secrets set itself carries no such notice (src/index.ts:164-166), and it does redeploy (src/commands/secrets.ts:192 prints ~ <serviceId> redeployed). A user who staged a mount path last week and runs insta secrets set FOO=bar today gets an unannounced remount and restart — while, per this PR's own copy, "application configuration is not updated automatically". Worth surfacing the warning at secrets set's help/output, or having the staging commands say that any deploy, including a secrets write, will apply the staged change.
3. Functionality — release ordering against the platform.
#504 is still open. Against platform main today, setVolumeSize returns early on sizeGib === undefined (changed:false) and never stages mountPath, and main's volumeView returns only {sizeGib, mountPath} — no pending/appliedMountPath. So insta compute volume web --mount-path /cache prints volume unchanged: 1Gi at /data: truthful about state, but the request is silently dropped with no hint it was ignored. The degradation is graceful (I checked the start-command path too — start_command is already on S.Service and in applySettings on main, so that half works today), but the CLI release should still be gated on #504 deploying.
4. Security — --set accepts credentials with no warning.
#504's own schema description for startCommand reads "Persisted as readable service settings; use secrets for credentials". The CLI help at src/index.ts:312 carries no equivalent, so insta compute start-command web --set 'exec app --token=…' writes a credential into readable service settings without a nudge. Mirroring that one clause into the --set option description would be enough.
Information
1. Issue #153's bullet "Allow environment variables and startup commands to change in the same deployment" is not delivered by the CLI — the PR explicitly routes that combination to Console, both in the body and in the help text. That reads as a deliberate scope call rather than an oversight, but since it is a literal line in the tracking issue it is worth an explicit sign-off rather than silent closure of #153 on this PR.
2. src/commands/compute.ts:670 — the ?? '(not deployed)' fallback is dead code: the spread is already gated on volume.appliedMountPath != null, so the nullish branch can never be taken.
3. src/commands/compute.ts:681 — the volume unchanged: … line is the only volume read/write line that omits the trailing (plan max NGi). Harmless, but it makes the no-op output inconsistent with every sibling line.
Performance
No findings. The path-only flow adds one extra GET .../volume before the PUT — a deliberate pre-flight, one round trip, no loop. No N+1, no unbounded iteration, no new blocking work.
Verdict
approved (informational — 0 Critical findings). My round-1 CHANGES_REQUESTED at bc29bcd is withdrawn on the merits; note that a COMMENT does not itself clear a standing changes-requested state, so a human still needs to give the explicit GitHub approval. Suggestion §1 is the one I would most like to see landed before merge — the guard is correct but nothing holds it in place.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Round 2 at 638c055. The r1 Critical (a path-only --mount-path on a volumeless service silently provisioning a plan-cap disk) is closed — both by the new client-side pre-check and by the guard that platform #504 now carries on setVolumeSize, the very route the CLI calls — and the four output arms I flagged as shadowed now render truthfully. No blocking findings remain; what is left is test binding, one no-op-truthfulness gap on the new start-command write, and docs/consistency notes.
Requirements context
instacloud-cli has no docs/superpowers/, no docs/ and no plans/ — no matching spec/plan found, so intent is taken from the PR body, the tracking issue InsForge/instacloud#153, and the companion PRs. Those companions were read directly:
- platform #504 — merged to
main2026-09-22T09:00Z (c58da22). Currentmainsrc/provisioning/services.ts:2508now hasif (mountPath !== undefined && sizeGib === undefined && service.volume_gib == null) throw new BadRequestError('no volume attached — provide sizeGib …')onsetVolumeSize(thePUT /services/:id/volumehandler), not only onapplySettings. That is the exact gap named in r1. - compute #291 — the remount lane;
deploy.tsnow selects the existing disk by identity andrecordAppliedVolumes the applied path. - instacloud-skills #120 (open) — the companion for CONTRIBUTING.md:42-44's "a command or flag change is only half done until it is mirrored in
insta/cli-reference.md" rule. It documentscompute start-commandand path-only volume edits and is release-gated on this PR. The convention is satisfied.
Verification run in a clean clone of 638c055: npm ci, npx tsc --noEmit (clean), npx vitest run — 84 files / 1716 passed | 1 skipped, matching the PR body.
Findings
Critical
(none)
r1 Critical — resolved, with evidence. src/commands/compute.ts:756-758 now reads the volume before writing and refuses a path-only edit on a volumeless service, and src/commands/compute.ts:729 rejects --size "" instead of letting it fall through to the read path. Negative controls in a copy of the head tree (88 tests across test/volume.test.ts, test/volume-mount-path.test.ts, test/compute-exec.test.ts):
| arm removed | result |
|---|---|
the !current.volume pre-check (compute.ts:757) |
1 failed ✔ bound |
the --size "" guard (compute.ts:729) |
1 failed ✔ bound |
&& volume.appliedMountPath != null in volumeLines (compute.ts:670) |
1 failed ✔ bound |
${sizeRequested ? 'grown to ' : ''} → always grown to (compute.ts:690) |
1 failed ✔ bound |
The r1 display Critical-adjacent Suggestion is also closed across all four arms I enumerated: a pre-deploy grow now prints volume grown to 20Gi at /data (was shadowed by the pending line), a post-deploy --size 20 --mount-path /x prints the grow and the pending remount, the changed === false no-op prints volume unchanged: 1Gi at /data, and a never-deployed volume (appliedMountPath: null, pending: true — volumeView sets pending: mountPath !== service.volume_applied_mount_path, so it is true for any undeployed volume) no longer claims a remount.
Suggestion
Software engineering — volumeWriteLine's pending-suffix guard is unpinned (src/commands/compute.ts:679, test/volume.test.ts:192-208). Relaxing body.volume.pending && body.volume.appliedMountPath != null to just body.volume.pending leaves all 88 tests green. The guard is not cosmetic: on a pre-deploy grow (insta compute volume web --size 20 against an attached-but-never-deployed volume) the backend returns {changed: true, attached: false, volume: {pending: true, appliedMountPath: null}}, and without the guard the line reads mount path null → /data pending. The existing "does not call a first attachment a remount" case uses attached: true, which returns from the earlier branch and never reaches this expression. Add a { volume: { sizeGib: 20, mountPath: '/data', appliedMountPath: null, pending: true }, cap, changed: true, attached: false } case asserting the line contains grown to 20Gi and no pending.
Software engineering — the sizeRequested call-site wiring is unpinned (src/commands/compute.ts:756, test/volume-mount-path.test.ts:39-43). Hardcoding the 4th argument to true at the call site also leaves all 88 green: test/volume.test.ts:205 exercises the renderer's false arm directly, and "sends a path-only edit without an implicit resize" only asserts the rawRequest payload, never the rendered output. Since "Path-only writes do not claim a grow" is a headline claim of the PR body, that claim currently has no end-to-end binding. Asserting info was called with a string not containing grown in that test would close it.
Functionality — start-command never reports a no-op (src/commands/compute.ts:2078-2081). The write ignores res.body.applied. Platform applySettings computes wantsCommand = patch.startCommand !== undefined && (patch.startCommand.trim() || null) !== (service.start_command ?? null) and only pushes 'startCommand' into applied when that is true — so re-setting the command already in effect (or --clear on a service that already runs the image default) returns applied: [] and the CLI still prints Startup command saved. The PR makes truthful no-op output an explicit goal for volumes (changed === false → volume unchanged: …); the start-command path deserves the same, e.g. branch on res.body.applied?.includes('startCommand').
Security — the help text drops the platform's own credential warning (src/index.ts:311, src/commands/compute.ts:2078). The platform schema for this field states "Persisted as readable service settings; use secrets for credentials", S.Service.start_command is returned by the plain service list (so any project member can read it back), and audit only stores a startCommandHash. The CLI help documents sh -c but not the readability, so --set 'app --password hunter2' is an easy footgun. Adding "stored as readable service settings — use insta secrets for credentials" to the start-command description would match this repo's otherwise very explicit help-text style.
Software engineering — README's command table was not updated (README.md:222). The insta compute row enumerates every other subcommand (I diffed it against src/index.ts: start-command is the only member of the code's set missing from it). CONTRIBUTING's cross-repo cli-reference.md rule is met by skills#120, but this table is in-repo.
Software engineering — start-command --json read shape (src/commands/compute.ts:2074). printJson({ service: svc }) dumps the whole service row and forces consumers to reach for the snake_case service.start_command, whereas sibling read commands narrow (src/commands/github.ts:210,251 emit { service: { id, name }, source }) and the write path emits the platform body ({service, applied, …}) — so read and write JSON disagree. { service: { id, name }, startCommand: svc.start_command ?? null } would be stable and consistent.
Information
- Unreachable conjunct (
src/commands/compute.ts:681). Removing&& !body.attachedfrom thechanged === falsebranch leaves all 88 tests green, and it is genuinely dead: the only path insetVolumeSizethat setsattached: truealso returnschanged: true. Harmless, but it is defensive code no caller can trip. - Wording drift between the two renderers.
volumeLines(compute.ts:670) says "deploy to apply (restarts the service)" whilevolumeWriteLine(compute.ts:680) says "deploy or restart to apply". Both are correct — platformrestart()routes throughrunComputeImage→applyComputeImage, which carries both the new mount path andcmd: ['sh','-c', start_command]— but the read path omitsinsta compute restartas an option. --mount-path ""has no client-side guard while--size ""now does. On an existing volume the platform rejects it (attachmentMountPath: "mountPath must not be empty for an existing volume"), but at attach time (--size 10 --mount-path "")normalizeVolumeMountPath('')silently returns/data. Pre-existing, not introduced here.serviceVolumeis generic but the new error hardcodesinsta compute volume(compute.ts:757). Unreachable today — only the compute group registers--mount-path(src/index.ts:313); managed-DB and postgres volume commands do not — but${type}is in scope if you want the message to stay honest if that changes.- Release gating. skills#120 explicitly holds its merge until this PR, platform #504 and compute #291 are all released. Platform #504 is merged as of today; this CLI change needs a release before
insta@latestusers can follow that documentation.
Performance
No performance-relevant regressions. The only added work is one extra GET .../volume per path-only write (compute.ts:756), which is required to make the refusal client-side; it is skipped whenever --size is present. No new loops, no new dependencies, no blocking I/O added.
Verdict
approved — zero Critical findings. The r1 blocker is genuinely closed on both the client and the (now-merged) backend side, and the output arms render truthfully. The two unpinned-test items above are worth landing before merge since they guard exactly the behaviours the PR body advertises; the GitHub approval itself remains a human action.
2 participants
Support path-only changes on an existing compute volume and staged runtime startup commands. Refuse implicit path-only attachment, preserve truthful size/attach/grow/no-op output, and show pending remounts. CLI secrets writes still deploy immediately; Console supports the combined variables/path/command deployment.
Tracks https://github.com/InsForge/instacloud/issues/153
Details:
The platform's current grow response reports changed:true; the display now preserves that result rather than allowing pending-path copy to hide it.
Validation: typecheck, build, and full suite: 84 files / 1,717 tests passed.