Add coop restore --reprovision to reset an instance without re-typing its flags - #432
Add coop restore --reprovision to reset an instance without re-typing its flags#432evandowning wants to merge 9 commits into
coop restore --reprovision to reset an instance without re-typing its flags#432Conversation
hbrodin
left a comment
There was a problem hiding this comment.
13 findings inline.
Approach. The capability is justified, and I'd push back on the follow-up note framing restore + start as the root fix — that would break the documented commit/restore checkpoint recipe. The src/lib.rs comment suggests only a packaging change: coop restore --reprovision with an optional --image, rather than a seventh top-level verb.
Blocking, in my view:
- No
signal::check_shutdown()between the handler install andrestore_disk, so Ctrl-C during the stop still lets the wipe proceed. - The integration suite hasn't been run, and it's the only coverage for all 168 lines of
cmd_recreate(zero mutants in scope). The phase also passes--no-agents, leaving half the differentiating claim untested. docs/commands.mdgets no section for the new verb, thoughdocs/index.mdcalls it "everycoopsubcommand". Not commentable inline — the PR doesn't touch that file.
In files the diff doesn't touch, so not inline:
docs/commands.md:773anddocs/images-and-profiles.md:173still giverestore+startas the checkpoint recipe with no caveat — the pairing this PR says leaves an empty/workspace.docs/json-output-design.md:159enumerates every command in its mutations row and omits this one.cmd_restore's success message still points atcoop start, which is the trap.
Minor, not worth threads: the not-replayed caveat appears seven times (README, CHANGELOG, --help, two doc comments, an inline block, a test comment); the validate-before-swap rationale five times in one function; RecreateWorkspaceInputs is a product type encoding a sum (4 of 8 field combinations legal — an enum over WorkspaceSource would make the swap its own doc worries about a compile error); the CHANGELOG names BootMode::Restart, a crate-private enum, in user-facing copy; README/CHANGELOG/--help say "re-synced or re-cloned" but a Mount source shows a third wording; and recreate_confirmation's two positive-arm tests lack the cross-arm negatives its Mount test has.
Checked clean: no new dependencies; no trust-boundary issues (workspace.json host-owned, workspace_dir reaches tar via argv, git_repo escaped with the token on stdin, prompt::confirm fails closed off a TTY, proxy::stop clears the host capability token); provision_first_boot's mutation exclusion matches the file's policy, and a scoped sweep of the three extracted helpers ran 15 mutants, 15 caught; completions generate from Cli::command(); no phantom features; cargo test --lib passes 1071.
Coverage: correctness, design, conventions, security, tests, docs, comments. review-api-usage skipped — no Cargo.toml or external-crate call-site changes.
coop recreate to reset an instance without re-typing its flagscoop restore --reprovision to reset an instance without re-typing its flags
|
Thanks — all 13 inline findings and the review-body items are addressed across two commits. 027ec3d fixes the findings against the code as reviewed, so each inline reply describes the change in terms of 1e98646 then takes the packaging suggestion: the verb is gone and the capability is Blocking items from your review:
The other three "in files the diff doesn't touch" items are all fixed too: the checkpoint recipes in From the minor list I took: the repeated not-replayed caveat (now stated once in the docs, with I did not convert One thing to look at with fresh eyes: the |
|
5 findings posted inline — the must-fix subset from a re-review at Blocking
Worth fixing in the same pass
On the integration run. The description still says Checked and clean: Not posted, for the record, since they're judgment rather than defects: Happy to push any of these if it saves you a round-trip — the Coverage: correctness, design, conventions, security, tests, docs, comments. |
e389604 to
30e643e
Compare
hbrodin
left a comment
There was a problem hiding this comment.
1 finding posted inline.
One additional cross-file consistency finding cannot be placed inline because the affected lines are outside this PR diff: the rebuilt-image instructions in CHANGELOG.md:19, docs/getting-started.md:116, docs/codex-integration.md:151, and src/backend.rs:1842 still recommend plain restore (or restore + start). This PR establishes that path skips workspace synchronization and plugin installation for a base image. Please update those instructions to use coop restore <vm> --image <image> --reprovision; it accepts a running instance and leaves it running.
The repository-required Lima and Firecracker integration runs also remain the final pre-merge validation gate.
Replaces an instance's guest disk with a fresh copy of its image, then provisions it through the first-boot path so the workspace is re-synced or re-cloned, agents are re-bootstrapped, and plugins, marketplaces and MCP servers are reinstalled. `coop restore` already swaps the disk, but the follow-up `coop start` runs in BootMode::Restart, which skips the workspace sync and the plugin install on the assumption that both survived on the guest disk. After a disk swap neither did, so restore + start leaves an empty /workspace and no plugins. The instance keeps its name, index, IP, image, disk size, port forwards and guest env, including a devcontainer's containerEnv and forwardPorts. Extra --extra-mount directories, --exclude-git and a devcontainer's postCreateCommand are not replayed, because coop does not persist them; the help text, README and CHANGELOG say so. Everything host-side is read and validated before the disk is touched, so a missing workspace directory, an unknown --image or a host-port collision aborts with the instance intact. The port-collision probe binds each host port, so it runs after this instance's own forwards are torn down rather than colliding with them. To share the post-boot half with `coop up`, start_instance is split: the part after the VM is up moves into provision_first_boot, which both callers use. That function is scoped out of the mutation sweep (it drives SSH and IO); the two new pure helpers stay in scope and are unit-tested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correctness - Check `signal::check_shutdown()` between the stop and `restore_disk`. `install_handlers` only sets a sticky flag, so a SIGINT during the stop was recorded but not observed until after the wipe. - Reject a non-UTF-8 recorded workspace path before the swap. `StartOpts::workspace_dir` is `Option<&str>`, so such a path reached `provision_first_boot` as `display()`'s lossy rendering and failed there identically on every re-run — a recreate that could never finish. - Warn when the applied `devcontainer.json` changed, as `restart_instance` does. Without it the state file kept asserting "applied" while the guest was rebuilt from the image. - Give the host-port collision bail context: the instance is stopped by then, and the underlying message suggests `--forward-port`, which `recreate` does not expose. Reuse - Call `cmd_stop` for the stop/teardown/proxy sequence instead of restating it. The `as_running` probe error it swallows does not matter here: the `as_stopped` proof is what gates the disk swap. - Point `cmd_restore`'s success message at `recreate` when the image is a base image rather than a checkpoint. Docs - Add the `recreate` section to `docs/commands.md`, note the base-image caveat on the `commit`/`restore` recipe in `commands.md` and `images-and-profiles.md`, and list the verb in the `json-output-design.md` mutations row. - Shorten `--help` from one 393-character line to paragraphs under 103. - Correct `--extra-mount` (was `--mount`, a boolean flag) and drop the two suggested recoveries that cannot work. - Split the doc comment that had run together over `RecreateWorkspaceInputs`. Tests - Drop `--no-agents` from the integration recreate so the agent-bootstrap half of `provision_first_boot` runs, and assert the managed settings are rewritten while a seeded sentinel key is gone. - Make the wipe assertion a positive discriminator; it previously passed when the guest was unreachable. - Compare the guest's own root filesystem size across the recreate, which `coop status` (host image file size) cannot see. - Fail instead of `|| true` on the stop/start that select which recreate branch runs. - Extract `check_recreate_workspace_source` and unit-test it; add cross-arm negatives to the `recreate_confirmation` tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drops the seventh lifecycle verb. The capability is unchanged; only its packaging is. `coop restore` gains `--reprovision`, which provisions the replaced disk as a first boot and leaves the instance running instead of swapping the disk and stopping there. `--image` becomes optional under `--reprovision`, defaulting to the image the instance already records, and stays required otherwise so a bare `coop restore` is still not a destructive no-argument command. `-y`, `--no-agents` and `--no-prompt` carry `requires = "reprovision"`: none of them mean anything to a plain disk swap, and `-y` would otherwise promise a prompt that never appears. `RestoreOpts` carries a `RestoreMode` enum rather than a `reprovision` bool beside the three flags that only apply when it is set, so the illegal combination is unrepresentable in the handler as well as at the CLI boundary. `cmd_recreate` becomes the private `reprovision_instance`, which `cmd_restore` routes to; the plain restore path is unchanged. `.cargo/mutants.toml` names `reprovision_instance` explicitly — it is no longer a `cmd_*`, so the blanket dispatch pattern no longer covers it. The four pure helpers stay in scope, renamed with it. `coop restore --help` stays within the terminal width: the added `long_about` is short paragraphs, since coop's clap has no `wrap_help`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes the review findings from #432: - `.cargo/mutants.toml`: the `reprovision_instance` exclusion used single backslashes in a TOML basic string, so `\b` parsed as backspace (U+0008) and the pattern reached cargo-mutants as `\x08reprovision_instance\x08`, matching nothing. Eight mutants in that function were left unexcluded and uncaught, against a documented 0-missed baseline for `lifecycle.rs`. - `tests/integration.sh`: the `restore --reprovision` phase assumed a running instance, but the preceding phase ends with `coop stop`. Neither `coop shell` nor `coop exec` auto-starts, so both seeds and two assertions failed while two others could not fail. The phase now starts the instance first. - `tests/integration.sh`: the `bypassPermissions` and `~/.claude.json` checks were described as FirstBoot-vs-Restart discriminators. Both `write_managed_claude_settings` and `seed_claude_onboarding` run on every boot, so they only prove `bootstrap_agents` ran against the blank disk. The comments now say so and name the workspace re-sync as the mode discriminator. The FirstBoot-only marketplace/plugin/MCP install cannot be asserted from this phase, which reuses the main flow's config; the comment records why and points at `test_local_marketplace`. - `tests/integration.sh`: nothing covered the `refresh_ssh_config_if_present` call added to `provision_first_boot` — `test_ssh_config` removes the `coop-<name>` block long before this phase, so the call was a no-op. The phase now installs the alias, asserts it still connects after the reprovision, and cleans up. - The "not replayed" lists named `postCreateCommand`, which coop does not implement — it is reported as an unrecognised `devcontainer.json` key. The affected key is `postStartCommand`. Corrected in the `reprovision_instance` doc, `docs/commands.md` and the CHANGELOG. - Both pre-swap workspace guards offered recoveries that do not work: nothing re-points an existing instance's recorded workspace, so neither `coop up` nor `coop push --dir` re-associates a moved directory. They now name restoring the path or `coop destroy` plus a fresh `coop up`. - `reprovision_partial_message` now names the pre-swap disk size and the `coop resize` command that reapplies it. Nothing persists an instance's disk size, so a re-run after a failure between the swap and the re-grow measures the template-sized file and silently drops a prior `coop resize`. - `coop restore --help` printed paragraphs of up to 94 characters. coop's clap has no `wrap_help`, so each renders as one unwrapped line; the kept and not-replayed lists are dropped in favour of a pointer to `docs/commands.md`. Gates: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo test (1085 passing), bash -n tests/integration.sh. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings from a second review pass over #432, none of them behavior changes to the reprovision path itself: - `RestoreOpts` carried a single `Option<&ImageName>` for both modes, so a `DiskOnly` restore with no image was representable and `cmd_restore` absorbed it with `unwrap_or_else(|| inst.image.clone())` — a silent destructive disk wipe from the instance's own recorded image. The image moves into the variants: required on `DiskOnly`, optional on `ReprovisionOpts` where "keep the recorded image" is a real default. The residual `Option` is collapsed once at the clap boundary, which is the only place that knows `--image` is `required_unless_present`. This extends the reasoning already behind `RestoreMode` to the field the enum did not cover. - Fixed the intra-doc link on `cmd_restore`: it named `RestoreOpts::reprovision`, a field that does not exist. Now `RestoreMode::Reprovision`. Confirmed with `cargo doc --no-deps --document-private-items` that no unresolved link remains in the files this PR touches; the four that remain are pre-existing in `proxy.rs` and `guest_env_state.rs`. - The non-UTF-8 workspace guard justified itself only through `StartOpts::workspace_dir` being `Option<&str>`, which covers the `Workspace` arm but not `Mount`. The guard is right to reject both — `Mount::from_parts` also takes `&str`, so neither arm can carry the real path — but the comment did not say why it applies to mounts. - `coop restore --help` claimed "a stopped instance" while `--reprovision` accepts a running one and stops it itself, and the `-y` help still rendered at 87 columns once clap's 10-space indent is counted. Both fixed and measured against the built binary: no line of `coop restore --help` now exceeds 80 columns. `docs/commands.md` and the README command table record the running-instance difference. Gates: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo test (1085 passing), bash -n tests/integration.sh, and `coop restore --help` rendered from the built binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings from a self-review pass over the two preceding commits, all in the test harness plus one CHANGELOG clause. - `test_restore_reprovision` started the instance unconditionally, but `coop start` is not idempotent: `find_stopped_instance` bails "already running". The previous phase's trailing stop is `|| true`, so a transient stop failure there would make the new guard fail and skip every assertion in the phase. It now probes `coop status` first and starts only when needed. - `restore --reprovision` had end-to-end coverage for only one of the three workspace sources. `$INSTANCE` uses the copy transport, so the `Mount` and `GitRepo` arms of `reprovision_workspace_inputs` were exercised only as a pure function by unit tests. `test_host_mount` and `test_git_repo` already build instances with those sources, so each now reprovisions its instance and asserts the disk was wiped (a guest-home sentinel is gone) and the workspace came back — the mount still serves the host directory, or the repository was re-cloned. Mount is the arm that needed it: `create_and_start` receives the mount set and `start_existing` does not, so a reprovisioned mount instance depends on the backend's own mount declaration being re-established against a wiped disk, which nothing verified. Both new reprovisions pass `--no-agents` so the subject stays the workspace source; the main phase covers the agent-bootstrap half. No assertion on `/data` in the git-repo phase: `--extra-mount` is not replayed by coop, but whether the mount point itself survives is backend dependent, so asserting either way would be wrong on one of them. - The SSH-alias assertion was vacuous on Firecracker, where the guest IP and port 22 are stable across stop/start, so an unrefreshed block still connects. The phase now rewrites that block's `Port` to 1 before the reprovision, so the connection afterwards succeeds only if `refresh_ssh_config_if_present` actually rewrote it. The rewriting step exits non-zero unless it changed exactly one `Port` line inside this instance's block, so a marker-format change cannot make the assertion silently vacuous again. - The CHANGELOG did not mention that `--reprovision` accepts a running instance, which the README table and `docs/commands.md` already state. Gates: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo test (1085 passing), bash -n tests/integration.sh, and `cargo mutants -f src/commands/lifecycle.rs -- --lib` reports 96 mutants, 84 caught, 12 unviable, 0 missed. The new shell is unrun: shellcheck is not available here and the integration suite still needs a pass on both backends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`docs/commands.md` and the `reprovision_workspace_inputs` doc both stated that an instance created with `--extra-mount /host/data:/data` "comes back without `/data`" after a reprovision. That holds on Firecracker, where a mount is a one-time sync into the rootfs and the data goes with the replaced disk. It does not follow on Lima: `restore_disk` copies the base image over the instance's disk and never touches `lima.yaml`, and `start_existing` shells out to `limactl start`, which re-reads that file including its `mounts:` section — so the directory may well be served again. Both now say coop replays none of them and describe the divergence, without promising an outcome on either backend. The integration test added alongside this deliberately asserts nothing about `/data` for the same reason. Also renames the mount phase's marker file from `reprovision-marker.txt` to `mount-remount-marker.txt`. The main reprovision phase uses `/workspace/reprovision-marker`, so the two differed only by suffix — close enough that a prefix match confused them while verifying the phase logic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every failure after the disk swap carries `reprovision_partial_message`, which told the user to re-run `coop restore <name> --reprovision` with no `--image`. That advice is wrong when `set_image` is the step that failed: `instance.json` still records the previous image, so the defaulted re-run targets that one and wipes the disk back to the wrong template instead of finishing the reprovision that was asked for. Pass the resolved image into the message and emit it explicitly. The closure now borrows `image` for the rest of the function, so `set_image` takes a clone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
30e643e to
6705f3f
Compare
Four places tell the user to pick up a rebuilt image with a plain `coop restore <vm> --image <image>`: the Codex account-auth CHANGELOG entry, the getting-started and codex-integration guides, and the Secret-Service bail in `backend.rs`. That is a base-image swap, and this PR establishes that the follow-up `coop start` skips the workspace sync and the plugin install, so the recipe leaves an empty `/workspace` and no plugins. Name `--reprovision` in all four. It also accepts a running instance and leaves it running, so the codex-integration recipe collapses from stop/restore/start to one command. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed in 2bf570e — thanks, this one was a real gap and I missed it on the first pass. All four sites came in with #438 (Codex ChatGPT account auth), which landed on
Your reasoning is exactly the case this PR exists for: those are base-image swaps, so the follow-up Because I swept the rest of the repo for the same pattern rather than just fixing the four you listed. The remaining plain- Gates at this head: The branch is rebased onto current |
Adds
coop restore --reprovision: replace an instance's guest filesystem with afresh copy of its image and bring it back with the settings it already had.
Why
coop destroy+coop uploses every per-instance state file —vm_config.json(vCPUs/memory),
forwards.json,guest_env.json,model.json,proxy.json,devcontainer_state.json— so every flag has to be re-typed.coop restoreswaps the disk while keeping that state, but the follow-upcoop startruns inBootMode::Restart, which deliberately skips the workspacesync and the plugin/marketplace/MCP install because those normally survive on
the guest disk. That is correct for a
coop commitcheckpoint — the restoreddisk already carries them, and overwriting them would defeat the rollback. It is
wrong for a base image, where neither survived:
restore+startthereleaves an empty
/workspaceand no plugins.--reprovisionis that second case. It runs the first-boot path instead.What it does
Validates everything host-side first, then: tears down the instance's port
forwards, stops it, stops the credential proxy, replaces the disk from the
recorded image, re-grows it to the size the instance had, and re-provisions
through the first-boot path. The instance is left running, so no follow-up
coop startis needed.Kept: name, index, IP, image, disk size, port forwards, guest env — including a
devcontainer's
containerEnvandforwardPorts. Features stay baked into theimage. GitHub PATs and provider credentials live in the host-side secret store
and are untouched.
Not replayed, because coop does not persist them: extra
--extra-mountdirectories,
--exclude-git, and a devcontainer'spostCreateCommand. The--helptext, README, CHANGELOG anddocs/commands.mdstate this rather thanimplying a full restore.
CLI
Plain
coop restoreis unchanged. The additions:--imagebecomes optional only under--reprovision, defaulting to theimage the instance already records. It stays required otherwise, so a bare
coop restoreis still not a destructive no-argument command.-y,--no-agentsand--no-promptcarryrequires = "reprovision". Noneof them mean anything to a plain disk swap, and
-ywould otherwise advertisea prompt that never appears.
-yis required off a TTY:prompt::confirmreturns false there, so ascripted reprovision without it fails closed.
RestoreOptscarries aRestoreModeenum rather than areprovisionboolbeside the three flags that only apply when it is set, so the illegal
combination is unrepresentable in the handler as well as at the clap boundary.
Ordering
Everything that can fail cheaply happens while the instance is still intact: the
image check, the state-file loads, the recorded workspace directory's existence
and UTF-8 validity, and
Mount::from_partscanonicalization. The host-portcollision probe binds each port, so it runs after this instance's own forwards
are torn down — probing earlier would collide with the instance being
reprovisioned — but still before the irreversible disk swap, and its bail names
the now-stopped state. A
signal::check_shutdown()sits immediately beforerestore_disk, so a Ctrl-C during the stop is acted on rather than onlyrecorded. A stopped instance can be started again; a wiped one cannot be
un-wiped. A failure after the swap is contextualized with the state it leaves
and the fact that re-running finishes the job.
Refactor
start_instanceis split so the post-boot half (provision_first_boot) isshared with
reprovision_instance. The extracted body is unchanged apart fromVec→slice, therepo.as_ref()moving to the call site, and one addedcall:
refresh_ssh_config_if_present, whichrestart_instancealready didand a first boot did not. A reprovision restarts an existing instance whose
forwarded port changes across stop/start on Lima, so it needs the refresh; the
call is a no-op for a freshly allocated instance, which cannot have a
coop-<name>block yet (workspace.rsgates onmarker_block_present). Itsdoc comment lists exactly which
StartOptsfields it reads, since thereprovision path passes placeholders for the creation-only ones.
reprovision_instancecallscmd_stopfor the stop / forward-teardown /proxy::stopsequence rather than restating it. Theas_runningprobe errorcmd_stopswallows is not load-bearing: theas_stoppedproof is what gatesthe disk swap.
Mountis built throughMount::from_partsrather than a struct literal: thepath comes from
workspace.json, and on Firecracker mounts are re-synced withrsync, which parses its own source argument.Testing
requires = "reprovision"rejections,
check_reprovision_workspace_source,reprovision_workspace_inputs,reprovision_confirmation). Full suite: 1077passing.
gone (the wipe happened) while a host-written marker appears in
/workspace(the re-sync happened) — the pair
restore+startcannot satisfy — plusguest env preserved, IP preserved, disk size preserved across the swap,
unknown
--imagerejected, and a non-TTY run without-yrefusing andleaving the guest untouched. The phase runs without
--no-agents, so theagent-bootstrap half of
provision_first_bootis exercised too: asettings.jsonsentinel seeded before the wipe must be gone while the managedpermissions block is back, the opposite of what
test_claude_settings_mergepins for a restart. It also compares the guest's own root filesystem size
(
df -Pk /) across the reprovision, whichcoop statuscannot see — thatnumber is the host image file's size.
.cargo/mutants.tomlscopes outprovision_first_bootandreprovision_instance(both drive SSH/IO; the latter is no longer acmd_*,so the blanket dispatch pattern no longer covers it) and leaves all four pure
helpers in scope. Mutation-verified by hand, since
cargo-mutantswas notavailable on the host this ran from: swapping
workspace_dir/git_repo,dropping the
guest_pathpassthrough, removing the data-loss wording, and —on
check_reprovision_workspace_source— flipping!is_dir(), flippingto_str().is_none(), and replacing the whole body withOk(())each fail atest.
Gates run:
cargo fmt --check,cargo clippy --all-targets --all-features -D warnings,cargo test,bash -n tests/integration.sh.Not yet run:
./tests/run-integration.shon either backend. This is alifecycle change, so it needs a pass on both Lima and Firecracker before merge.
Two of the added assertions cannot be pre-verified from here — in particular the
guest-filesystem check below.
Follow-ups (deliberately not in this PR)
(
restart_instance,start_instance,reprovision_instance). Converging itis a change to the instance state model, not a local edit.
workspace::check_guest_dirtyexists andis used by
push/pull; a reprovision that warned about uncommitted guestwork would need it plus a
--force.StartOpts::workspace_dirisOption<&str>, so a non-UTF-8 workspace pathround-trips lossily through
Path::display(). The reprovision path nowrejects such a path before the swap (it would otherwise bail after the disk
was gone, on every re-run), but widening the field to
Option<&Path>is whatremoves the class.
resize_disktruncates and relies on cloud-initgrowpart; since thedisk was just replaced by a base image with a completed cloud-init run, the
re-grow may not reach the guest filesystem. Pre-existing
coop resizemechanics that the reprovision path now depends on. The integration phase now
asserts this directly with a guest-side
df, so the Lima run is what settlesit; if it fails, the fix belongs in
resize_disk, not the test.