Skip to content

Add coop restore --reprovision to reset an instance without re-typing its flags - #432

Open
evandowning wants to merge 9 commits into
mainfrom
add-recreate-command
Open

Add coop restore --reprovision to reset an instance without re-typing its flags#432
evandowning wants to merge 9 commits into
mainfrom
add-recreate-command

Conversation

@evandowning

@evandowning evandowning commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Adds coop restore --reprovision: replace an instance's guest filesystem with a
fresh copy of its image and bring it back with the settings it already had.

Why

coop destroy + coop up loses 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 restore swaps the disk while keeping that state, but the follow-up
coop start runs in BootMode::Restart, which deliberately skips the workspace
sync and the plugin/marketplace/MCP install because those normally survive on
the guest disk. That is correct for a coop commit checkpoint — the restored
disk already carries them, and overwriting them would defeat the rollback. It is
wrong for a base image, where neither survived: restore + start there
leaves an empty /workspace and no plugins.

--reprovision is 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 start is needed.

Kept: name, index, IP, image, disk size, port forwards, guest env — including a
devcontainer's containerEnv and forwardPorts. Features stay baked into the
image. 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-mount
directories, --exclude-git, and a devcontainer's postCreateCommand. The
--help text, README, CHANGELOG and docs/commands.md state this rather than
implying a full restore.

CLI

coop restore [NAME] [--image <name>] [--reprovision] [-y] [--no-agents] [--no-prompt]

Plain coop restore is unchanged. The additions:

  • --image becomes optional only under --reprovision, defaulting to the
    image the instance already records. It 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 advertise
    a prompt that never appears.
  • -y is required off a TTY: prompt::confirm returns false there, so a
    scripted reprovision without it fails closed.

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 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_parts canonicalization. The host-port
collision 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 before
restore_disk, so a Ctrl-C during the stop is acted on rather than only
recorded. 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_instance is split so the post-boot half (provision_first_boot) is
shared with reprovision_instance. The extracted body is unchanged apart from
Vec→slice, the repo.as_ref() moving to the call site, and one added
call
: refresh_ssh_config_if_present, which restart_instance already did
and 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.rs gates on marker_block_present). Its
doc comment lists exactly which StartOpts fields it reads, since the
reprovision path passes placeholders for the creation-only ones.

reprovision_instance calls cmd_stop for the stop / forward-teardown /
proxy::stop sequence rather than restating it. The as_running probe error
cmd_stop swallows is not load-bearing: the as_stopped proof is what gates
the disk swap.

Mount is built through Mount::from_parts rather than a struct literal: the
path comes from workspace.json, and on Firecracker mounts are re-synced with
rsync, which parses its own source argument.

Testing

  • 20 new unit tests (CLI parsing including the requires = "reprovision"
    rejections, check_reprovision_workspace_source,
    reprovision_workspace_inputs, reprovision_confirmation). Full suite: 1077
    passing.
  • New integration phase asserting the load-bearing claims: a guest sentinel is
    gone (the wipe happened) while a host-written marker appears in /workspace
    (the re-sync happened) — the pair restore + start cannot satisfy — plus
    guest env preserved, IP preserved, disk size preserved across the swap,
    unknown --image rejected, and a non-TTY run without -y refusing and
    leaving the guest untouched. The phase runs without --no-agents, so the
    agent-bootstrap half of provision_first_boot is exercised too: a
    settings.json sentinel seeded before the wipe must be gone while the managed
    permissions block is back, the opposite of what test_claude_settings_merge
    pins for a restart. It also compares the guest's own root filesystem size
    (df -Pk /) across the reprovision, which coop status cannot see — that
    number is the host image file's size.
  • .cargo/mutants.toml scopes out provision_first_boot and
    reprovision_instance (both drive SSH/IO; the latter is no longer a cmd_*,
    so the blanket dispatch pattern no longer covers it) and leaves all four pure
    helpers in scope. Mutation-verified by hand, since cargo-mutants was not
    available on the host this ran from: swapping workspace_dir/git_repo,
    dropping the guest_path passthrough, removing the data-loss wording, and —
    on check_reprovision_workspace_source — flipping !is_dir(), flipping
    to_str().is_none(), and replacing the whole body with Ok(()) each fail a
    test.

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.sh on either backend. This is a
lifecycle 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)

  • The saved-forwards / guest-env reload logic now exists in three places
    (restart_instance, start_instance, reprovision_instance). Converging it
    is a change to the instance state model, not a local edit.
  • No dirty-guest check before the wipe. workspace::check_guest_dirty exists and
    is used by push/pull; a reprovision that warned about uncommitted guest
    work would need it plus a --force.
  • StartOpts::workspace_dir is Option<&str>, so a non-UTF-8 workspace path
    round-trips lossily through Path::display(). The reprovision path now
    rejects 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 what
    removes the class.
  • On Lima, resize_disk truncates and relies on cloud-init growpart; since the
    disk 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 resize
    mechanics 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 settles
    it; if it fails, the fix belongs in resize_disk, not the test.

@evandowning evandowning self-assigned this Aug 28, 2026
@evandowning evandowning added the enhancement ✨ New feature or request label Aug 28, 2026
@evandowning
evandowning marked this pull request as ready for review August 28, 2026 16:18

@hbrodin hbrodin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and restore_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.md gets no section for the new verb, though docs/index.md calls it "every coop subcommand". Not commentable inline — the PR doesn't touch that file.

In files the diff doesn't touch, so not inline:

  • docs/commands.md:773 and docs/images-and-profiles.md:173 still give restore + start as the checkpoint recipe with no caveat — the pairing this PR says leaves an empty /workspace.
  • docs/json-output-design.md:159 enumerates every command in its mutations row and omits this one.
  • cmd_restore's success message still points at coop 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.

Comment thread src/lib.rs Outdated
Comment thread src/commands/lifecycle.rs
Comment thread src/commands/lifecycle.rs Outdated
Comment thread src/commands/lifecycle.rs
Comment thread src/commands/lifecycle.rs
Comment thread src/commands/lifecycle.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread tests/integration.sh
Comment thread tests/integration.sh Outdated
Comment thread src/commands/lifecycle.rs
@evandowning evandowning changed the title Add coop recreate to reset an instance without re-typing its flags Add coop restore --reprovision to reset an instance without re-typing its flags Aug 31, 2026
@evandowning

Copy link
Copy Markdown
Contributor Author

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 coop recreate.

1e98646 then takes the packaging suggestion: the verb is gone and the capability is coop restore --reprovision. Nothing about the earlier fixes changed, only their names — read the inline replies with that substitution. The PR title and description are rewritten to match.

Blocking items from your review:

  • signal::check_shutdown() before the wipe — fixed. It now sits between the stop and restore_disk.
  • docs/commands.md section — added, covering both restore modes plus a comparison table against destroy + up and restore + start.
  • Integration suite not run — still true, and still the blocking item. I have no VM host here. The phase now runs without --no-agents and asserts on the agent bootstrap, so it covers the half you flagged as untested, but it needs a real pass on both Lima and Firecracker before merge.

The other three "in files the diff doesn't touch" items are all fixed too: the checkpoint recipes in docs/commands.md and docs/images-and-profiles.md now carry the base-image caveat, docs/json-output-design.md's mutations row is corrected, and cmd_restore's success message names --reprovision instead of steering everyone into the trap.

From the minor list I took: the repeated not-replayed caveat (now stated once in the docs, with --help and the doc comments pointing at it rather than restating it), the repeated validate-before-swap rationale (stated once as an ordering rule), the BootMode::Restart reference in user-facing CHANGELOG copy, the "re-synced or re-cloned" wording that ignored the mount arm, and the missing cross-arm negatives on reprovision_confirmation's two positive tests.

I did not convert RecreateWorkspaceInputs to an enum over WorkspaceSource. You flagged it as not worth a thread and the struct is now ReprovisionWorkspaceInputs with the same shape; happy to do it if you would rather it land here than as a follow-up.

One thing to look at with fresh eyes: the df assertion added for the disk re-grow may well fail on Lima, for the reason the description's follow-up already gave. If it does, that is the real defect surfacing and the fix belongs in resize_disk, not in the test.

@evandowning
evandowning requested a review from hbrodin August 31, 2026 13:33
Comment thread .cargo/mutants.toml Outdated
Comment thread tests/integration.sh Outdated
Comment thread src/commands/lifecycle.rs Outdated
Comment thread src/commands/lifecycle.rs Outdated
Comment thread src/commands/lifecycle.rs
@hbrodin

hbrodin commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

5 findings posted inline — the must-fix subset from a re-review at b2c9406, after each one was put through an adversarial pass that tried to falsify it. Everything that didn't survive that pass has been dropped rather than posted.

Blocking

  • .cargo/mutants.toml:188 — the \b escaping makes the exclusion inert; cargo mutants reports 8 missed and exits 2 against the 0-missed baseline in docs/testing.md. One-character fix.
  • tests/integration.sh:2214 — the phase runs against a stopped instance, so it produces four hard failures and two assertions that cannot fail.

Worth fixing in the same pass

  • postCreateCommand should be postStartCommand in four places.
  • The non-UTF-8 workspace message recommends a recovery that doesn't exist.
  • A prior coop resize is silently dropped if a reprovision fails between the disk swap and the re-grow.

On the integration run. The description still says ./tests/run-integration.sh hasn't been run on either backend, and CLAUDE.md requires both for a lifecycle change. Those two items are coupled: the suite as written will fail when it runs, so the tests/integration.sh fix has to land before a run can tell you anything. The Lima run is also what settles the open question in your own follow-up notes about whether the re-grow reaches the guest filesystem — test_restore_reprovision's df -Pk / check is aimed squarely at it, though note its floor is never asserted to have grown, so today it would pass if neither the resize nor the re-grow reached the guest.

Checked and clean: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo test (1077 passed), taplo format --check all pass at this head. No trust-model issues — the credential proxy's capability token dies with the disk and is reminted by the first-boot bootstrap, state files are host-owned, and Mount::from_parts canonicalizes the one path that reaches a host-side command. No new dependencies. The four new pure helpers are in mutation scope with 17/17 mutants caught. b2c9406 is a clean catch-up merge with main — every PR-owned file is byte-identical to 1e98646.

Not posted, for the record, since they're judgment rather than defects: reprovision_instance still restates the restore_disk/resize_disk/forwards/guest-env blocks that cmd_restore/cmd_resize/restart_instance own; provision_first_boot takes a whole StartOpts but reads 8 of its 14 fields, with the contract held by a doc paragraph rather than the type; its SSH-readiness prefix is a near-copy of restart_instance:1109-1129 whose comments have already drifted apart; --extra-mount is described as not replayed, which holds on Firecracker but not on Lima, where the mount lives in the per-instance lima-template.yaml and survives the swap; docs/ARCHITECTURE.md:136 still names lifecycle.rs:start_instance as owning the whole first-boot sequence after the split; and docs/testing.md's named registry has no entry for the reprovision_* helpers.

Happy to push any of these if it saves you a round-trip — the mutants.toml one-liner and the postCreateCommand rename in particular are mechanical, and I can do the integration-phase fix plus the plugin/MCP assertion if you'd rather review a diff than write one. Just say which.

Coverage: correctness, design, conventions, security, tests, docs, comments. review-api-usage skipped — no Cargo.toml/Cargo.lock or external-crate call-site changes in the PR's own diff.

@hbrodin hbrodin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/commands/lifecycle.rs Outdated
evandowning and others added 8 commits September 3, 2026 07:37
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>
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>
@evandowning

Copy link
Copy Markdown
Contributor Author

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 main after this branch was cut, so they were genuinely outside the diff until I rebased. They are in the tree now and all four are fixed:

File Was Now
CHANGELOG.md:19 coop restore <vm> --image <image> coop restore <vm> --image <image> --reprovision
docs/getting-started.md:116 coop restore <vm> --image <image> coop restore <vm> --image <image> --reprovision
docs/codex-integration.md:150 coop stop / coop restore --image default / coop start coop restore my-project --image default --reprovision
src/backend.rs:1842 `coop stop` then `coop restore <vm> --image <image>` coop restore <vm> --image <image> --reprovision

Your reasoning is exactly the case this PR exists for: those are base-image swaps, so the follow-up coop start takes BootMode::Restart and skips both the workspace sync and the plugin install — the recipe as written hands you an empty /workspace and no plugins, which is the worst possible outcome for someone just trying to pick up a rebuilt image.

Because --reprovision accepts a running instance and leaves it running, the codex-integration.md recipe collapses from three commands to one. I also rewrote the prose under it to say why: it provisions the replaced disk as a first boot, so /workspace is restored and plugins are reinstalled, where a plain restore would leave both empty.

I swept the rest of the repo for the same pattern rather than just fixing the four you listed. The remaining plain-restore references are all legitimate checkpoint rollbacks (docs/commands.md:787, docs/images-and-profiles.md:189, and the historical #289 CHANGELOG entry), where the restored disk does carry the workspace and plugins and skipping the re-sync is the correct behavior. docs/images-and-profiles.md:195 already spells out the base-image distinction, so I left those alone.

Gates at this head: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo test (1136 passing) all clean. No test asserted on the backend.rs bail text, so nothing needed updating alongside it.

The branch is rebased onto current main (13b67be). And agreed on the last point — the Lima and Firecracker integration runs are still the outstanding pre-merge gate; I still have no VM host here, so that remains the blocking item on this PR rather than something I can close out.

@evandowning
evandowning requested a review from hbrodin September 3, 2026 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement ✨ New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants