Skip to content

feat(acq): add msb native snapshot restore - #507

Draft
mogul wants to merge 13 commits into
feat/default-opencode-devenv-basefrom
feat/native-snapshot-restore
Draft

mogul wants to merge 13 commits into
feat/default-opencode-devenv-basefrom
feat/native-snapshot-restore

Conversation

@mogul

@mogul mogul commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Context

Stack child of draft PR #584, which moves default OpenCode provisioning to the immutable generic devenv base plus the required pinned agent kit on both backends. This PR adds native msb-backed snapshot/restore and lifecycle helpers for preserving agent context across planned recreate/reboot flows.

Changes

  • Adds acq snapshot, acq restore, acq recreate, and acq rm --snapshot.
  • Uses msb native full-state snapshots and restore-time resource handling.
  • Keeps sbx unsupported for stateful snapshot/restore rather than offering disk-only degraded state semantics.
  • Re-derives the current host SSH-agent vsock route at restore.
  • Uses private per-invocation archive staging to prevent concurrent restore races.
  • Adds ADR-0036 and dispatch, lifecycle, Windows host-path, secret re-export, and recreate coverage.

Stack Dependency

This PR is based on #584 (feat(opencode): use generic devenv base by default). Review and merge the base PR first. This PR intentionally remains draft until its own required live MSB verification is complete and a human requests review.

Verification

  • PASS: focused rebased Bats: ./scripts/test-acq-bats test/bats/70-msb-backend.bats test/bats/117-msys-path-forms.bats (1..43).
  • PASS: bash -n acq acq.backends/common.sh acq.backends/msb.sh acq.backends/sbx.sh scripts/verify-native-snapshot-restore.
  • PASS: ShellCheck repository batch (--severity=warning).
  • PASS: Markdown lint, executable-bit validation, and git diff --check.
  • PASS: base PR feat(opencode): use generic devenv base by default #584 full offline suite (740 tests) and npm run check before stacking.
  • BLOCKED: rerunning the complete stacked ./scripts/test-acq-bats and npm run check exceeded this environment's 10-minute command limit while Bats was still executing; no test failure was emitted.

Live Validation Required

On an MSB/KVM-capable host, run:

scripts/verify-native-snapshot-restore [SANDBOX] [WORKSPACE]

It verifies archive creation, newest/explicit restore selection, recreate, and SSH signing via the re-derived vsock route. Also run the base PR live checks before either draft is marked ready:

./scripts/verify-backends --only sbx
./scripts/verify-backends --only msb

Rollback

Revert this PR after its base PR is reverted or absent. It adds acq-owned stateful lifecycle verbs only; sbx remains unsupported for them.

Security Impact

MSB-only local lifecycle behavior changes. Stored secrets are exported only transiently around restore then unset. No secret values are exposed, and restore validates archive metadata and resource handling.

AI Disclosure

AI-assisted changes require human review before deployment. This PR remains draft until explicitly marked ready by the user or a human reviewer.

@mogul

mogul commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Update from follow-up verification pass:

  • Pushed commit 193e739 (test(acq): make snapshot verifier non-interactive).
  • The live verifier now creates the test sandbox non-interactively, writes a guest-state sentinel before snapshot, and verifies that sentinel after primary restore, explicit copy restore, and acq recreate.
  • It no longer opens an interactive shell at the end.
  • It documents that a USAi key must already be stored/exported because the verifier is now non-interactive.
  • Also tightened the msb test stub so msb inspect NAME fails for absent sandboxes unless a test explicitly provides inspect output, matching the new acq_backend_exists fallback contract.

Verification:

  • PASS: bash -n acq acq.backends/msb.sh scripts/test-acq-lib.sh scripts/verify-native-snapshot-restore
  • PASS: shellcheck --severity=warning acq.backends/msb.sh scripts/test-acq-lib.sh scripts/verify-native-snapshot-restore
  • PASS: git diff --check
  • PASS: ./scripts/test-acq-bats test/bats/70-msb-backend.bats test/bats/110-volumes.bats test/bats/116-clone-option.bats (1..61)
  • PASS: ./scripts/test-acq-bats (1..519)
  • PASS: host-side VERIFY_SNAPSHOT_CLEANUP=1 ./scripts/verify-native-snapshot-restore on Bret's macOS/msb host. Observed guest sentinel restored after primary restore, copy restore, and recreate; restored SSH-agent route reachable in all restore paths.

The GitHub scoped-token advisory still prints during create when no sandbox-scoped token exists, but because the verifier runs with stdin redirected it is advisory-only and does not block.

@mogul
mogul marked this pull request as ready for review September 24, 2026 02:41
@mogul
mogul requested a review from a team as a code owner September 24, 2026 02:41
@wz-gsa

wz-gsa commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head 6289ba9, main at 30f9ecf. Leading with one cross-PR blocker; I'll follow up on the substance separately.

Blocker: ADR-0030 is claimed twice, and the whole 0030/0031 window is contended

This PR adds docs/adr/0030-native-snapshot-restore.md. #504 also adds docs/adr/0030-host-authoritative-sandbox-config.md. Neither touches the other's file, so git will merge both cleanly and leave two different ADR-0030s in docs/adr/. Every "ADR-0030" reference in either PR's code comments, docs, and issue threads then becomes ambiguous — and #504's is already cited as the mechanism of record from a second repo (patterns #423's ADR-0003 and its Layer 1 design point at it by number).

Audited every open quickstart PR, and it's tangled further than a single pair:

PR ADR it adds State
#507 (this) 0030-native-snapshot-restore collides with #504
#504 0030-host-authoritative-sandbox-config collides with #507; cited cross-repo by number
#500 0031-interactive-acq-configure already renumbered 0028→0031 to dodge a different collision
#510 (none yet) but #504's own comment says #510 "will take ADR-0031" — which #500 has already committed

main currently tops out at 0029-windows-host-guest-path-forms.md, so 0030 and 0031 are both double-claimed and 0032 is the first genuinely free number.

Worth noting this is the second instance of the same class in three days — #500 originally added a second 0028 while 0028-windows-native-secret-storage was already in main, for the same reason: the branch point predated the merge, nothing conflicts textually, and no CI check looks at ADR numbering.

Two things would help, in order:

  1. Someone picks the winner for 0030 and the other renumbers. My read: docs(adr): add ADR-0035 host-authoritative sandbox configuration #504 should keep it, because its number is already referenced from another repository, and a cross-repo citation is more expensive to fix than a local one. That puts this PR at 0031 — except feat(acq): interactive acq configure UI for extra kits + token-scoping defaults #500 holds that, so this would be 0032, and fix(msb): block unsafe 0.7 migration versions #510 takes 0033 when it needs one.
  2. A CI check that fails when a PR adds an docs/adr/NNNN- prefix that already exists on the base branch or in another open PR. Three collisions in one window is a process gap, not bad luck. The check is a few lines of ls docs/adr/ | cut -c1-4 | sort | uniq -d against the merge base, plus a gh pr list sweep for the cross-PR case.

I'd rather raise this now than after two 0030s land, since untangling references after the fact costs more than renaming a file.


Method: compared docs/adr/ contents at each open PR's head against origin/main, and enumerated ADR additions via gh pr diff … | rg -o 'b/docs/adr/…' for #500/#504/#510. No PR code executed. The substance of the snapshot/restore design is not covered here — I'll come back to it once the numbering is settled, so a re-read isn't wasted on a file that may move.

@basiliskus

Copy link
Copy Markdown
Contributor

Reviewed at 6289ba9.

Adds acq-owned snapshot / restore / recreate / rm --snapshot verbs backed by msb's native full-state snapshot primitive, with acq-owned re-plumbing of workspace volume mounts and the host SSH-agent vsock route at restore time. sbx fails closed on all four rather than degrading to a disk-only template flow.

Intent: Matches the stated intent. The two findings below are gaps within that intent, not a different problem.

Findings (2)

  1. Blocking: Unguarded empty-array expansion aborts provision on bash 3.2. _restore_volume_specs is appended to only inside the _ws_recs loop, so it is empty whenever no workspace positional is given (acq create opencode, acq run opencode — derive_name falls back to basename $PWD and preflight_workspace_path returns 0 on an empty arg, so both reach provision). Reproduced under real bash 3.2.57 with a stubbed msb: acq.backends/msb.sh: line 3379: _restore_volume_specs[@]: unbound variable. The abort lands after msb create succeeded and the guest became exec-ready, stranding a half-provisioned sandbox with no kits and a raw interpreter error instead of an acq diagnostic. bash 5.x tolerates the bare form, so this only bites macOS system-bash users. Every other new array expansion in the diff is guarded; acq.backends/sbx.sh:1140 states the rule ("\"${arr[@]}\" on an empty array is a fatal "unbound variable" under set -u on bash 3.2 (the macOS system bash)"), and .github/workflows/bash32-compat.yml gates on acq.backends/** — but no offline test provisions without a workspace path, so the gate does not catch it. Fix is ${_restore_volume_specs[@]+"${_restore_volume_specs[@]}"}.
    acq.backends/msb.sh:3379
  2. Blocking: restore and recreate never re-run kit startup, so a restored sandbox comes up with kit-supervised services dead. Both arms end at acq_backend_restore, which re-plumbs exec-readiness, OCI devices, and the ssh-agent bridge — but not kits. Every other path that brings a sandbox back up (start at acq:1329/1333, restart at acq:1362/1363, both run re-attach branches at acq:1063/1065 and acq:1155/1157) pairs acq_cli_kits_load with acq_backend_ensure_kits_applied for exactly this reason. This matters more here than on resume: any sandbox created with a workspace positional records a volume line, so restore takes the --disk-only --external-mount-policy relaxed cold-boot branch (acq.backends/msb.sh:1144, asserted in test/bats/70-msb-backend.bats:95) — no memory state, supervisors gone. ADR-0017 is explicit that msb replays only runtime.entrypoint/runtime.cmd, "never from runtime.scripts", and the start) comment says the heal is "the actual, deterministic mechanism that brings supervised services back up". The result is KNOWN_FAILURE_MODES chore(ci): bump the actions group with 2 updates #33 ("ports mapped, nothing listening"), reached via the feat(ci): add minimal pre-commit config (opt-in) for secret detection #34 guidance this PR adds recommending acq recreate. acq prints a reassuring "recreated '$name' from snapshot" either way, and the live verifier only checks listing + an on-disk sentinel + ssh-add -l, so it would not catch this.
    acq:1268, acq:1298

Below the bar: (a) rm --snapshot calls acq_backend_terminate (cleanup mode) where recreate correctly calls acq_backend_terminate_for_restore, so for --clone sandboxes or opt-in kits declaring volumes: the archive it produces is not cleanly restorable — the PR's own comment at acq.backends/msb.sh:4796 is the authority that this "would destroy the resources restore needs"; scored 70 only because no default-kit configuration is affected (acq:1397). (b) The new snapshot dir is created without chmod 700/umask 077, unlike the msb-startup and --clone state-tree dirs; weaker than it looks because msb keeps real secrets out of the guest, but it breaks the file's own convention (acq:527, acq.backends/msb.sh:1043). (c) scripts/verify-native-snapshot-restore is committed 100644 while every sibling verifier is 100755; no hook catches it, and there is pre-existing precedent for the same slip in scripts/.

Verdict: needs changes

Assisted-by: OpenCode [claude-opus-5] team-code-review

@wz-gsa

wz-gsa commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Narrowing my earlier comment — part of it is now stale, and the remaining collision is smaller than I described. AI-assisted, advisory.

#510 has taken ADR-0032, not 0031. Verified in its diff at 28265b8: it adds docs/adr/0032-msb-version-policy-and-migration-recovery.md. So the 0031 half of what I raised has resolved itself — #500 holds 0031 uncontested, and #510 is clear of both.

Re-checked all four just now, against main still topping out at 0029:

PR ADR it adds
#504 0030-host-authoritative-sandbox-config
#507 (this) 0030-native-snapshot-restore
#500 0031-interactive-acq-configure — uncontested
#510 0032-msb-version-policy-and-migration-recovery — uncontested

So one collision remains: 0030, between this PR and #504. Neither touches the other's file, so git will merge both cleanly and leave two different ADR-0030s in docs/adr/.

My read is unchanged on which should move: #504 keeps 0030, because its number is already cited from another repository — agentic-coding-patterns' ADR-0004 names it as the mechanism of record for host-materialized config — and a cross-repo citation costs more to fix than a local rename. That puts this PR at 0033 now that 0031 and 0032 are both taken.

The CI-check suggestion from my earlier comment still stands and is arguably worth more now that three of the four have sorted themselves out by hand: comparing added docs/adr/NNNN- prefixes against the merge base and against other open PRs would have caught all of this at the point the second PR opened, rather than after four PRs and two renames. Happy to draft it if that's useful.

Still holding off on the substance of the snapshot/restore design until the number settles — reviewing a doc that may be renamed wastes the re-read, and this comment is only about the filename.

@wz-gsa

wz-gsa commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at 6289ba9. Patch offered, not pushed — this is your branch and your merge decision.

I independently confirmed both of @basiliskus's blocking findings (measured, not read), prototyped fixes for both plus the test gap that let the first one through, and ran them under real bash 3.2. Everything below is offered inline so you can take, alter, or reject it.


Blocker 1 — reproduced on real bash 3.2.57

acq.backends/msb.sh:3379 bare-expands "${_restore_volume_specs[@]}". That array is appended to only at :3227, inside the _ws_recs loop — which iterates zero times when no workspace positional is given.

I transcribed the exact shape and ran it on both interpreters:

bash 3.2.57  ->  line 12: _restore_volume_specs[@]: unbound variable   rc=1
bash 5.3.20  ->  ARG:[]  SURVIVED                                      rc=0

Two details that make this narrower and more fixable than it looks:

  • The loop six lines above already uses the guarded form — ${_ws_recs[@]+"${!_ws_recs[@]}"} at :3188. The idiom was known and applied, then dropped at the call site.
  • The callee already handles zero args. _acq_msb_restore_resources_write:1046 does for _spec in ${@+"$@"}. So the caller was the sole gap — no callee change needed.
  # Guarded expansion: _restore_volume_specs is appended to only inside the
  # _ws_recs loop above, so it is EMPTY whenever no workspace positional was
  # given (`acq create opencode`, `acq run opencode`). A bare "${arr[@]}" on an
  # empty array is a fatal "unbound variable" under `set -u` on bash 3.2 (the
  # macOS system bash) -- and it would abort here, after `msb create` already
  # succeeded, stranding a half-provisioned sandbox. Same rule as sbx.sh:1140-1142.
  _acq_msb_restore_resources_write "$name" \
    ${_restore_volume_specs[@]+"${_restore_volume_specs[@]}"}

Blocker 2 — the PR contains its own indictment

restore) ends at acq_backend_restore (acq:1268); recreate) ends at it plus an echo (:1298). Neither re-drives kits.

The start) arm thirty lines below explains why that's required:

after the backend resume we re-drive acq_backend_ensure_kits_applied: it re-applies the pinned kits idempotently and RE-RUNS the startup phase via the exec path — the actual, deterministic mechanism that brings supervised services back up

And I confirmed the cold-boot path at msb.sh:1144: any recorded workspace volume adds --disk-only --external-mount-policy relaxed, so there is no memory state and supervisors are gone. Since KNOWN_FAILURE_MODES #34 (added by this PR) points users at acq recreate to escape #33, that arm in particular routes people into #33.

The fix is the same pair start) and restart) already use, copied verbatim rather than invented — for restore):

    acq_backend_restore "$snapshot_ref" --name "$local_name"
    # Re-run kit startup after the restore, for the same reason `start` does
    # (ADR-0017): a restore that carries a recorded workspace `volume` takes the
    # --disk-only cold-boot branch (acq.backends/msb.sh:1144), so there is no
    # memory state and kit-supervised services are dead. Without this the
    # sandbox comes back with ports mapped and nothing listening
    # (KNOWN_FAILURE_MODES #33). No-op for a sandbox with no kit record.
    acq_cli_kits_load "${ACQ_RESOLVED_BACKEND}" "$local_name"
    acq_backend_ensure_kits_applied "$local_name" || true
    echo "acq: restored '$local_name' and re-applied kit startup." >&2

and the same three lines after acq_backend_restore in recreate), with the message becoming ... (kit startup re-applied).

Checked for sbx safety: acq_cli_kits_load lives in common.sh (both backends), and both arms already exit 1 on sbx before reaching it, so this cannot affect the sbx path.

The message changes are deliberate — they give the tests a positive observable to assert on rather than only checking for absence.

The test gap that let blocker 1 through

bash32-compat.yml builds bash 3.2.57 from source and runs the full bats suite under it, so the gate should have caught this. It didn't, because no test provisions without a workspace path. Three tests added to test/bats/70-msb-backend.bats:

Test Covers
create with NO workspace positional provisions without aborting (bash 3.2) blocker 1 — the gate's blind spot
restore re-applies kit startup (no dead supervisors) blocker 2
recreate re-applies kit startup (no dead supervisors) blocker 2, the #34 path specifically

Full patch: +70/−2 across acq, acq.backends/msb.sh, test/bats/70-msb-backend.bats. Happy to attach it however is easiest — a patch file, a branch you can cherry-pick, or a PR into feat/native-snapshot-restore if you'd prefer that.


How I ran the tests, and why that matters

All three pass under real bash 3.2.57 — but the invocation matters, because on my host a naive bats run is a silent no-op:

$ /bin/bash test/vendor/bats-core/bin/bats test/bats/70-msb-backend.bats
$ echo $?
0                      # ...and ZERO bytes of output. Not a pass — a non-run.

bash -x shows bats' last line is exec env BATS_ROOT=... bats, and on this machine command -v env resolves to a comment-only ~/.local/bin/env stub that runs nothing and exits 0. That is a local-host problem, not a repo defect — but it means an exit-0 bats run can be a non-run, so I'm stating the exact command rather than just claiming green:

$ PATH="/usr/bin:$PATH" /bin/bash test/vendor/bats-core/bin/bats --tap test/bats/70-msb-backend.bats
ok 10 msb: create with NO workspace positional provisions without aborting (bash 3.2)
ok 11 msb: restore re-applies kit startup (no dead supervisors)
ok 12 msb: recreate re-applies kit startup (no dead supervisors)

Separately: two of this PR's own new tests are red on macOS

This is not pre-existing debt — I checked:

origin/main                      70-msb-backend.bats:  16 ok,  0 failures
this PR (changes stashed)        70-msb-backend.bats:  21 ok,  2 failures

The two failures are msb: restore maps to msb restore with inherited resources and re-derived vsock (:95) and msb: recreate snapshots, removes, then restores to the same name (:141) — both new in this PR, and failing with my changes removed, so they aren't caused by anything I did.

Cause: the --vsock path is canonicalized to /private/var/folders/... while $STUBDIR is the symlinked /var/folders/... form, and the assertions expect the symlinked one. Every other path in the same assertion matches; only --vsock diverges.

expected: --vsock /var/folders/…/agent.sock:3552/stream
actual:   --vsock /private/var/folders/…/agent.sock:3552/stream

All five workflows are ubuntu-latest, where /var is not a symlink — so CI cannot see this, including the bash32-compat gate. Net effect: a macOS contributor running the suite locally gets two red tests that CI reports green, on exactly the paths this PR adds. Likely fix is asserting against a canonicalized $STUBDIR in those two expectations, or canonicalizing once in setup().

Worth noting this is the same shape as blocker 1 — a real failure the gate structurally cannot observe.


Method: static analysis at 6289ba9, plus transcription-and-run of the empty-array expansion on bash 3.2.57 and 5.3.20, and the bats suite under 3.2.57 with the invocation above. No PR code was executed against a live msb, and nothing was pushed to your branch. UNVERIFIED: the live behaviour of the kit heal after a real msb restore — scripts/verify-native-snapshot-restore is what would establish that, and I did not run it.

@mogul

mogul commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

AI-assisted update pushed in bc94b02. Please re-review.

Feedback Addressed

  • Renumbered the native snapshot/restore ADR from ADR-0030 to ADR-0033 and updated durable references.
  • Fixed the bash 3.2 / set -u empty-array abort in msb no-workspace provision.
  • Made restore/recreate re-run kit startup only for disk-only restored snapshots that need cold-boot healing, avoiding duplicate startup on full-state restores.
  • Made disk-only restore heal fail closed instead of claiming success when kit reapply fails.
  • Added .kits sidecar copy/adoption so explicit restore-to-copy keeps persisted CLI/extra kit refs before heal.
  • Preserved restore-needed derived resources for rm --snapshot, matching recreate.
  • Hardened acq-owned snapshot/restore metadata directories with private permissions and fail-closed behavior where sidecar metadata must be written/adopted.
  • Made scripts/verify-native-snapshot-restore executable.
  • Adjusted msb Bats coverage for no-workspace create, disk-only restore heal, full-state restore no-heal, restore-to-copy kit sidecar adoption, macOS canonicalized vsock paths, snapshot dir permissions, and rm --snapshot resource preservation.

Verification

PASS: bash -n acq acq.backends/common.sh acq.backends/msb.sh acq.backends/sbx.sh scripts/test-acq-lib.sh scripts/verify-native-snapshot-restore
PASS: ./scripts/test-acq-bats test/bats/70-msb-backend.bats (1..24)
PASS: ./scripts/test-acq-bats (1..528)
PASS: printf '%s\\n' acq acq.backends/*.sh scripts/test-acq-lib.sh scripts/test-acq-bats test/bats/*.bats test/bats/helper.bash | xargs -r -n8 shellcheck --severity=warning\nPASS: npm run lint\nPASS: git diff --check\nBLOCKED: npm run check (pre-commit is not installed in this environment: sh: 1: pre-commit: not found)\n```\n\n**Adversarial Review**\n\nRan two sub-agent review passes after implementation. The first found valid lifecycle/sidecar/fail-closed issues; those were fixed. The final pass reported no findings.\n\n**Residual Risk**\n\nLive `msb` snapshot/restore was not run here because this environment is not confirmed KVM/HVF-capable. Real backend restore behavior and SSH-agent vsock re-plumbing still need host-side live validation.

@basiliskus basiliskus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: request changes — b5b6efa

The next step should be rebase onto current main, then re-review from scratch. This is not just marker cleanup: acq and acq.backends/msb.sh have semantic conflicts with the v4 kit/provenance architecture. docs/BACKEND_GUIDE.md is the only mechanical conflict.

  • Blocking — acq.backends/msb.sh:1068-1080: restore trusts ${snapshot}.resources and passes its volume entries directly to msb restore --volume. A crafted sidecar beside an obtained snapshot can mount an arbitrary host path into the restored guest. Validate bindings against acq-owned source metadata or reject untrusted sidecars.
  • Blocking — acq:520-551, acq:1235-1324, acq.backends/msb.sh:1028-1180,2813-3480: rebase semantically. In acq, snapshot helpers collide with new secret helpers and rm must retain the new version guard. In msb.sh, workspace mount recording collides with current host-workspace provenance and the agent-kit/merged-env provisioning rewrite; restore also calls _acq_msb_grant_oci_devs, which main deleted. In docs/BACKEND_GUIDE.md, retain main's agent-kit language and add the snapshot text.
  • Blocking — acq:524-532, acq.backends/msb.sh:1094-1101: full-state archives may contain credentials, but only the default directory is chmodded. Set a restrictive umask for archive creation and verify/chmod the archive and sidecars, including explicit output paths. Current tests check only the directory mode.
  • Blocking — acq.backends/msb.sh:1172-1180: a restore that never becomes exec-ready only warns and then reports success. Fail closed and add a regression test for the timeout/error path.
  • Non-blocking — scripts/verify-native-snapshot-restore:1: the verifier is mode 100644; the documented ./scripts/verify-native-snapshot-restore command returns Permission denied.
  • Note — Prior Bash 3.2 blocker: the code now uses the guarded ${array[@]+...} expansion at acq.backends/msb.sh:3385-3386, and test/bats/70-msb-backend.bats:91-97 adds the missing no-workspace regression path. Real Bash 3.2 execution remains unverified here because only Bash 5.3.9 is installed.
  • Note — The other bc94b02 claims are present: full-state restore skips kit heal; disk-only heal failure propagates; .kits is copied/adopted for restore-to-copy. The fail-closed heal path has no explicit failure regression test.
  • Note — ADR renumbering is consistent: this PR adds only docs/adr/0036-native-snapshot-restore.md; main has no 0035/0036, open PR #504 claims 0035, no other open PR claims 0036, and no stale ADR-0030/0033 reference to this decision remains.
  • Note — There are no CI checks on this head. Local results: syntax PASS; diff-check PASS; ShellCheck PASS; Markdown lint PASS (50 files, 0 issues); npm run check BLOCKED (pre-commit: not found); Bats BLOCKED (vendored submodules absent; branch currently contains 528 @tests, not the claimed 519); live msb and Bash 3.2 BLOCKED by unavailable runtimes.

Assisted-by: OpenCode [gpt-5.6-sol-latest-guardrails-defaultv2] skill:change-review

@wz-gsa

wz-gsa commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head b5b6efa.

I closed your residual risk. You noted:

"Live msb snapshot/restore was not run here because this environment is not confirmed KVM/HVF-capable."

This host is capable, so I ran the live cycle — eight sandboxes, two snapshots, all removed afterwards.

$ msb doctor
  ✓ libkrunfw   /opt/homebrew/.../libexec/libkrunfw.5.dylib
  ✓ Root clone  reflink supported
  ✓ Host setup is ready.            macOS aarch64, HVF via libkrun, msb 0.7.7

Headline: your restore-mode branch is correct and is better than my original Blocker 2 — but the comment justifying it describes a failure mode msb 0.7.7 does not produce, and the real behaviour is worse for the design than the comment implies.


Confirmed fixed

Blocker 1 — msb.sh:3386 now carries the guarded expansion, with a || return 1:

${_restore_volume_specs[@]+"${_restore_volume_specs[@]}"} || return 1

Blocker 2, and your refinement beats my finding. I had said restore) and recreate) must re-drive kits, full stop. You made it conditional on the restore being disk-only. That is right, and I can now show why my version would have been wrong:

no external mount, source boot_id=a1317d99 uptime=696

full-state restore   uptime_s=784   boot_id=a1317d99  SAME   -> genuinely RESUMED
  (reproduced)       uptime_s=759   boot_id=a1317d99  SAME
disk-only restore    uptime_s=0     boot_id=52f8552f  NEW    -> COLD BOOT

A full-state restore really does resume the VM — same boot id, uptime continuing past the snapshot point. Supervisors survive. Re-running kit startup there, as my unconditional version would have, would double-start them. Branching on mode is the correct shape.


Finding — the --disk-only justification does not reproduce, and the real behaviour is silent

msb.sh:1142-1147 says:

"Full memory/device restore currently fails for exported snapshots that contain an external virtio-fs workspace mount (virtio_fs device-state restore reports ENOENT)."

Tested against a sandbox with a real bind mount (2 virtiofs mounts confirmed in-guest), source boot_id=ab55fe42 uptime=480:

full restore, NO bindings:
  error: invalid config: restore requires destination bindings for: filesystem /work;
         provide --volume mappings, select captured disks with --volume GUEST,
         or explicitly use --allow-missing-resources

full restore + CORRECT --volume binding:
  rc=0,  0 bytes stdout,  0 bytes stderr
  uptime_s=73   boot_id=d194b966  NEW    -> SILENTLY COLD-BOOTED
  (reproduced: uptime_s=71, boot_id=436b3cb5)
  /work/marker.txt still readable — disk preserved

Neither is an ENOENT. Without bindings it refuses at config validation, before any device-state restore is attempted, and names three remedies. With correct bindings it succeeds, says nothing, and cold-boots anyway.

I checked that the silence is real rather than me truncating output — rc=0 with zero bytes on both streams. And msb does error loudly on a bad binding, so this is not general sloppiness:

full restore + bad binding (control):
  error: failed to start "pr507ctl"
    -> other: restore cannot add uncaptured filesystem transports      rc=1

Why this matters beyond the comment text

The heal path's correctness rests entirely on _acq_msb_restore_resource_flags_into detecting every recorded resource, because that detection alone sets ACQ_BACKEND_RESTORE_NEEDS_KIT_HEAL=1. If it ever misses a workspace mount:

  • acq takes the full-restore path,
  • msb silently cold-boots rather than erroring,
  • the heal flag stays 0, so _acq_restore_heal_kits_if_needed returns early,
  • kits are never re-driven — dead supervisors, no healing.

That is exactly Blocker 2's failure mode, reached through a detection gap instead of a missing branch. Had msb failed as the comment claims, such a gap would be loud. Because it degrades silently, it is invisible.

So the comment is not just inaccurate — it describes a backstop that does not exist, which makes the detection look less load-bearing than it is.

Two things I would suggest, neither large:

  1. Correct the comment to what msb actually does, and say plainly that a detection miss fails silently. That is the sentence a future reader needs.
  2. Consider asserting the mode rather than inferring it. acq already computes --volume specs; after a restore it could check whether the sandbox actually resumed (a boot_id/uptime probe is one cheap way) and heal on the observed outcome instead of the predicted one. That would make a detection gap self-correcting rather than silent. Your call on whether that is worth the complexity — the current logic is correct when detection is complete.

One point in the PR's favour, measured

acq deliberately does not use msb's third remedy, --allow-missing-resources. That is the right call:

full restore + --allow-missing-resources:
  rc=0 BUT warn: external mount /work: no trusted destination mapping is
       available; filesystem operations return EIO (stale inodes: [])
  uptime_s=47  boot_id=1927346d  NEW
  /work -> "ls: can't open '/work': I/O error"

It cold-boots and hands the agent a workspace that returns EIO on every operation. Choosing --disk-only with re-bound resources over this is clearly correct, and worth keeping as a comment so nobody "simplifies" toward the flag that looks more permissive.


Method, and one trap

Eight sandboxes (pr507live, pr507full-r, pr507disk-r, pr507ws, pr507ws-full-r, pr507ws-full-r2, pr507ctl, pr507ctl2, pr507amr) and two snapshots, all removed — msb list and msb snapshot list report zero remaining.

A trap worth knowing if you reproduce this: a backgrounded process is not a usable memory marker. I planted sleep 600 & intending to check survival across restore, then verified it first — kill -0 reported GONE, because every msb exec gets a fresh process namespace and the child dies with the exec. Relying on it would have shown "full-state restore lost memory" when it never held the process at all. boot_id + uptime are the markers that actually work.


Not re-checked this round

The SSH-agent vsock re-derivation at restore, which you also listed as needing host-side validation. I measured the restore-mode and resource-binding behaviour because that is what the blockers turned on; the vsock path needs a sandbox with a live agent forward and is a separate exercise. Happy to do it if useful.

Also noting #507 is currently DIRTY against main, so it needs a conflict resolution pass regardless — but none of the above depends on that, since it is msb behaviour rather than merge state.

@mogul
mogul force-pushed the feat/native-snapshot-restore branch from b5b6efa to bb388df Compare October 8, 2026 07:45
@mogul

mogul commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

AI-assisted update: rebased onto current main and force-pushed bb388df (fix(acq): harden snapshot restore metadata). Please re-review from this head.

Blocking feedback addressed

  • Rebased the feature through the v4 agent-kit/provenance changes. The resulting branch retains current secret helpers, the rm version guard, merged kit-env behavior, and current agent-kit documentation.
  • Removed trust in archive-adjacent .resources / .kits metadata. acq now records resources and CLI/extra-kit refs in private ACQ_MSB_SNAPSHOT_DIR metadata keyed by the archive SHA-256.
  • Stages a selected archive privately before both digest lookup and msb restore, so the bytes trusted for host bindings are the bytes msb restores. The temporary staged archive is removed after restore or any restore/adoption failure.
  • Keeps restore-to-copy kit healing: trusted kit references are adopted under the target sandbox name before disk-only kit heal.
  • Creates snapshot archives under umask 077, requires 0600 on explicit archives, and keeps trusted metadata directories/files 0700/0600.
  • Restore now fails closed if the guest never becomes exec-ready. The readiness probe also preserves the actual msb exec failure status instead of masking it through the output-normalization pipeline.
  • Corrected the msb 0.7.7 restore rationale: valid virtio-fs rebindings may silently cold-boot, so acq intentionally uses disk-only restore plus kit healing for trusted external resources.
  • Made scripts/verify-native-snapshot-restore executable and retained the MSYS host-path conversion contract for its staged restore input.

Regression coverage added/updated

  • Archive-adjacent resource metadata cannot inject a host --volume.
  • Disk-only restore-to-copy adopts trusted kit refs and re-applies startup.
  • No-workspace provision remains Bash 3.2-safe.
  • Restore readiness timeout returns failure and emits no false success.
  • Explicit archive permissions, SHA-256 metadata lookup, and MSYS staged-path conversion are asserted.

Verification

PASS  bash -n acq acq.backends/common.sh acq.backends/msb.sh acq.backends/sbx.sh scripts/test-acq-lib.sh scripts/verify-native-snapshot-restore
PASS  ShellCheck repository batch (`--severity=warning`)
PASS  git diff --check
PASS  scripts/fix-exec-bits --check
PASS  ./scripts/test-acq-bats test/bats/70-msb-backend.bats (1..30)
PASS  ./scripts/test-acq-bats test/bats/117-msys-path-forms.bats (1..12)
PASS  ./scripts/test-acq-bats (1..732; three expected `sbx` grammar checks skipped because `sbx` is not installed)
BLOCKED npm run lint: `.github/linters/node_modules/.bin/markdownlint` is absent
BLOCKED gitleaks scan: `gitleaks` is not installed
BLOCKED live msb verification: this Linux environment is not confirmed KVM-capable

I ran four adversarial sub-agent review passes during this update. They identified the metadata trust, digest collision, kit-provenance, executable-bit, TOCTOU, cleanup, and MSYS path-form issues above; each confirmed issue was fixed before this push.

AI-generated changes require human review before deployment.

@basiliskus basiliskus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: request changes — bb388df

I previously reviewed the old head b5b6efad in review 5430255938. The rebase and follow-up commit resolved all five findings from that review:

  1. Resolved: restore no longer trusts archive-adjacent resource or kit sidecars. It privately stages the selected archive and keys acq-owned 0600 metadata by its SHA-256 (acq.backends/msb.sh:1039-1134, 1192-1236).
  2. Resolved: the rebase removed the deleted _acq_msb_grant_oci_devs seam while preserving main's secret helpers, version guard, merged kit environment behavior, and agent-kit documentation.
  3. Resolved: explicit archives are created under umask 077, must accept mode 0600, and have a mode assertion (acq.backends/msb.sh:1147-1155; test/bats/70-msb-backend.bats:92-96).
  4. Resolved: restore now fails closed when exec readiness times out, and the probe preserves the real msb exec status (acq.backends/msb.sh:1237-1240, 1266-1272).
  5. Resolved: scripts/verify-native-snapshot-restore is mode 100755.

Two issues remain:

  1. Major: make staged restore paths unique. acq.backends/msb.sh:1056-1058 maps identical archive bytes to the same ${sha256}.msb, while each invocation unconditionally removes that shared path at 1228-1236. Concurrent restores of the same archive can therefore overwrite or delete one another's input before msb restore opens it. Keep a per-invocation private 0600 staging name and use the content digest only for metadata lookup; add a collision/concurrency regression test.

  2. Minor: correct the durable restore rationale. docs/adr/0036-native-snapshot-restore.md:44-48 still says full device-state restore with exported virtio-fs mounts "fails." The live result supplied in GSA-TTS/agentic-coding-quickstart#507 was rc=0 with a silent cold boot, which the production comment now accurately states at acq.backends/msb.sh:1202-1206. Update the ADR to describe that observed behavior.

Assisted-by: OpenCode [gpt-5.6-sol-latest-guardrails-defaultv2] skill:change-review

@mogul

mogul commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

AI-assisted update pushed in bad0401 (fix(acq): isolate concurrent snapshot restores). Please re-review.

Feedback addressed

  • Restore staging now keeps a private, per-invocation mktemp archive. The SHA-256 digest remains only the key for trusted acq-owned metadata, eliminating the shared ${digest}.msb race where concurrent same-archive restores could remove or overwrite each other's input.
  • Added regression coverage proving two stages of identical archive bytes receive distinct paths and remain independent when one is removed.
  • Updated existing restore-command and MSYS host-path assertions for the intentionally unique .restore.XXXXXX staging path.
  • Corrected ADR-0036: valid exported virtio-fs rebindings can succeed while silently cold-booting; disk-only restore makes that behavior and kit healing explicit.

Verification

PASS: bash -n acq acq.backends/common.sh acq.backends/msb.sh acq.backends/sbx.sh scripts/test-acq-lib.sh scripts/test-acq-bats scripts/verify-native-snapshot-restore
PASS: focused Bats: test/bats/70-msb-backend.bats + test/bats/117-msys-path-forms.bats (1..43)
PASS: ./scripts/test-acq-bats (full offline suite; three expected sbx grammar checks skipped because sbx is unavailable)
PASS: ShellCheck repository batch (--severity=warning)
PASS: npm run check (including gitleaks, Markdown lint, ShellCheck, Bats, and executable-bit validation)
PASS: git diff --check

Live msb snapshot/restore remains host-capability dependent and was not run in this Linux workspace. AI-generated changes require human review before deployment.

@mogul
mogul marked this pull request as draft October 14, 2026 09:27
@mogul
mogul changed the base branch from main to feat/default-opencode-devenv-base October 14, 2026 09:39
@mogul
mogul added this pull request to stack #585 October 14, 2026 09:39
@mogul
mogul force-pushed the feat/native-snapshot-restore branch from bad0401 to 86b55d0 Compare October 14, 2026 09:39
mogul added 3 commits October 14, 2026 15:13
ADR-0033 was double-claimed with #519, which keeps 0033. Move this PR's native snapshot/restore ADR to the next free number.

Co-authored-by: OpenCode [gpt-5.5] <bret.mogilefsky@gsa.gov>
Rebase native snapshot restore on the v4 kit and provenance architecture. Keep restore metadata in private SHA-256-addressed state, stage archives before restore, and fail closed when readiness fails.

Co-authored-by: OpenCode [gpt-5.6-terra] <bret.mogilefsky@gsa.gov>
@mogul
mogul force-pushed the feat/native-snapshot-restore branch from 86b55d0 to 7d18d4f Compare October 14, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants