Skip to content

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

Open
mogul wants to merge 30 commits into
mainfrom
feat/native-snapshot-restore
Open

mogul wants to merge 30 commits into
mainfrom
feat/native-snapshot-restore

Conversation

@mogul

@mogul mogul commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Context

Adds MSB-native disk snapshot/restore and lifecycle helpers for preserving durable agent context across planned recreation or host reboot. PR #584, the generic OpenCode base dependency, is merged into main; this branch is rebased directly onto that merged base.

Changes

  • Adds acq snapshot, acq restore, acq recreate, and acq rm --snapshot.
  • Uses MSB native disk snapshots, the supported archive format for cold recovery with external workspace resource rebinding.
  • Reapplies kit startup/configuration after every restored disk snapshot.
  • Preserves snapshot-authenticated agent, workspace, generic OpenCode mode, and CLI/extra-kit state across restore, including legacy provenance-only sandboxes.
  • Makes acq rm --snapshot an export followed by permanent cleanup of derived volumes and live host metadata; digest-keyed snapshot sidecars remain available for restore.
  • Re-derives the current host SSH-agent vsock route at restore.
  • Uses private per-invocation archive staging to prevent concurrent restore races.
  • Uses a canonical host source for the trusted MSB host-config mount, fixing macOS /var -> /private/var provenance-mount failures.
  • Keeps SBX unsupported for acq snapshot/restore rather than exposing its incompatible template flow.

Rebase

Rebased onto main after #584 merged. The six previously inherited generic-OpenCode commits were intentionally omitted because their squash merge is already present in main; this resolves the GitHub merge conflict without dropping snapshot functionality.

Verification

  • PASS: focused MSB snapshot and dispatch tests (89 tests).
  • PASS: full npm run check, including gitleaks, Markdown lint, ShellCheck, complete offline Bats, and executable-bit validation.
  • PASS: Bash syntax, ShellCheck, and git diff --check.
  • PASS: adversarial review of the supported disk-snapshot correction found no blockers after one assertion update.
  • PASS: all GitHub CI checks at edb18da are green, including pre-commit and the bash 3.2 offline suite.
  • PASS: live macOS MSB snapshot verification with VERIFY_KEEP=1 ./scripts/verify-native-snapshot-restore. It created a disk snapshot, restored the newest archive by sandbox name, restored the explicit archive into a copy sandbox, preserved the guest sentinel in all restores, re-applied kits, re-derived reachable SSH-agent vsock forwarding, and completed acq recreate.
  • LIVE FINDING RESOLVED: a --full exported snapshot plus --disk-only failed because the installed MSB requires an internal snapshot reference not expressible by msb restore. The implementation now uses MSB’s supported default disk snapshot format and ordinary cold restore.

Retained Verification Sandboxes

The live verifier retained snaptest-opencode and snaptest-opencode-copy for inspection. Do not remove them without explicit authorization.

Rollback

Revert this PR to remove the MSB-only lifecycle verbs. SBX remains unsupported for sandbox snapshot/restore.

Security Impact

MSB-only local lifecycle behavior changes. Secrets remain transient around restore. Snapshot sidecars are local, digest-keyed, restrictive-permission metadata; they contain no secret values. Restored host state is validated and replaces stale target state.

AI Disclosure

AI-assisted changes require human review before deployment.

@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 3 times, most recently from 7d18d4f to 86dd0e6 Compare October 14, 2026 17:49
@mogul
mogul marked this pull request as ready for review October 15, 2026 00:16
@mogul
mogul force-pushed the feat/native-snapshot-restore branch from 4859865 to f2bf6e9 Compare October 15, 2026 10:43

@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 — f2bf6e9

scripts/verify-msb-host-config-mount:1 is documented with direct-execution usage, but the new file is committed as mode 100644. Consequently, ./scripts/verify-msb-host-config-mount exits 126 with Permission denied before running the verifier. Please commit it as executable (100755), consistent with the other scripts/verify-* entry points.

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

@mogul
mogul force-pushed the feat/native-snapshot-restore branch from f2bf6e9 to 20579dc Compare October 21, 2026 09:30
@mogul
mogul force-pushed the feat/native-snapshot-restore branch 2 times, most recently from c74b7e0 to d7ab52c Compare October 21, 2026 18:07
@mogul

mogul commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@basiliskus I found a pretty serious bug during host-side verification that meant I had to change how this works. Can you re-review?

@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 — edb18da6208b27570ea29d8daca5f3c5830ceed5

The CLI help still describes acq snapshot as preserving live state and explicitly promises disk, memory, execution, and device state (acq:94, acq:341-347). This revision intentionally creates a disk snapshot without --full and always cold-boots and heals kits on restore. The stale contract also appears in the recreate and rm --snapshot help, so users may remove a sandbox believing in-memory or running-process state is recoverable when it is not. Please update all shipped help text to consistently describe durable disk-state recovery.

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

mogul added 2 commits October 22, 2026 02:33
… conditional kit heal

The previous help text for acq snapshot, restore, recreate, and rm --snapshot
claimed that snapshots capture disk, memory, execution, and device state.
The implementation uses disk-only mode (when external workspace mounts are
present) and unconditional cold-boot restore; in-flight process state is
not preserved. Kit re-application via exec-heal is conditional on the
presence of acq-managed external workspace mounts, not unconditional.

Update all user-facing help text, usage table, error messages, ADR-0036,
ADR-0010 adapter contract, BACKEND_GUIDE.md, docs/howto/acq.md, and
docs/howto/msb.md to consistently describe:
- disk-state snapshot, not live/stateful/full-state snapshot
- cold-boot restore, not live-state recovery
- kit re-application conditional on external workspace mounts
- no claims of memory, execution, or device state continuity

Update focused Bats tests to assert accurate disk-state language and
refute 'live state', 'execution, and device' overclaims.

Co-authored-by: OpenCode [claude_4_6_sonnet] <bret.mogilefsky@gsa.gov>
…mic binding

Use 'local IFS' for Bash 3.2-safe dynamic scoping to preserve the caller's
IFS state, including when intentionally unset under 'set -u'. The local
declaration auto-restores the caller's IFS on function exit, avoiding
explicit snapshot/restore that fails when IFS is unset.

Add tests confirming IFS is preserved (unset stays unset, set value stays set)
for both the loaded-from-kit and fallback paths.

Co-authored-by: OpenCode [claude_4_6_sonnet] <bret.mogilefsky@gsa.gov>
@mogul
mogul force-pushed the feat/native-snapshot-restore branch from edb18da to f574921 Compare October 22, 2026 02:45
@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Review response — f574921

Addressed: CLI help text overclaiming memory/execution/device state continuity.

Changes in this update (2 new commits)

c9e9432 fix(acq): correct snapshot/restore help text — disk state, cold boot, conditional kit heal

All user-facing snapshot/restore text updated to accurately describe the implementation:

  • acq snapshot usage table + --help: "Save durable disk state" / "cold-boots from the saved disk image / in-flight process state is not preserved / kit re-application conditional on external workspace mounts"
  • acq restore --help: explicit cold-boot contract, no unconditional kit-heal claim
  • acq recreate --help: same corrections
  • acq rm --snapshot --help: "durable disk-state snapshot archive (cold-boot recoverable)"
  • Error messages: "does not support sandbox snapshots/restore/recreate" (removes "stateful")
  • ADR-0036: title, Context, Decision, Consequences rewritten — --disk-only is conditional on external workspace mounts, kit re-application is conditional, no claim of memory/execution/device continuity
  • ADR-0010 adapter contract: "disk-state snapshot" instead of "native full-state"
  • docs/BACKEND_GUIDE.md: 5 locations updated from "native full-state" to "disk-state"
  • docs/howto/acq.md: quick-reference table row fixed (was "live state")
  • docs/howto/msb.md: updated to match implementation
  • Bats tests in 30-dispatch-routing.bats and 70-msb-backend.bats updated to assert accurate disk-state language and refute "live state" / "execution, and device" claims

f574921 fix(provider-facts): preserve unset caller IFS with local-scoped dynamic binding

Carries forward the main-branch IFS fix (3fe3150, which landed on main after the PR branched) and its accompanying tests, so the local branch does not regress behind main on this fix.

Verification

  • PASS: bash -n acq acq.backends/msb.sh acq.backends/common.sh
  • PASS: ShellCheck (severity=warning) across all shell files
  • PASS: npm run lint:md (markdownlint)
  • PASS: focused Bats: test/bats/30-dispatch-routing.bats, test/bats/70-msb-backend.bats, test/bats/50-kit-list-completeness.bats
  • PASS: adversarial review (second pass) found no remaining blockers in the shipped help text

Not changed

The msb snapshot create --full flag is preserved in the implementation — the snapshot binary still captures disk state using msb's full-snapshot primitive. The help text correction is about what restore does, not what snapshot records. The implementation intentionally does not use msb's live-memory checkpoint restore path.

@mogul
mogul requested a review from basiliskus October 22, 2026 02:46

@wz-gsa wz-gsa 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.

AI-assisted review by OpenCode (usai/gpt-6-astra) of #507 at f574921. Three blocking findings are inline.

HEAD CHANGED DURING REVIEW: started at edb18da, then fetched and inspected the complete tree delta to f574921. This is not a documentation-only update: --full capture and conditional --disk-only restore/kit healing are back. The PR body's successful disk-snapshot live evidence describes edb18da, not these restored semantics. Re-ran the targeted tests and adversarial probes on f574921 before posting.

Verification on the reviewed head:

  • 70-msb-backend.bats on macOS Bash 3.2: 31 pass, 1 skip (timeout executable unavailable), 0 failures.
  • Independent probes sourced production functions and stubbed backend/kit effects only: failed extra-kit apply returns success through the real restore-heal chain; actual cleanup removes a synthetic clone workspace while digest-keyed restore metadata retains its now-missing bind source. Probe files and synthetic state were isolated in the approved temporary directory.
  • Bash syntax, ShellCheck on acq/common/msb, and git diff --check pass.
  • Real installed msb help confirms default capture is disk and --full adds memory/execution state; no live sandbox mutation performed.
  • Earlier wider run on edb18da encountered two MSYS assertions and timed out after 120 seconds during the readonly-kit suite. Their baseline attribution was not established, so they are not findings or claimed passing evidence.

Additional non-blocking observations:

  • scripts/verify-msb-host-config-mount is still mode 100644 at the reviewed tree, and direct execution returned Permission denied. This previously reported executable-bit issue appears to have returned.
  • Missing digest-keyed state/kit/resource sidecars return success without clearing corresponding name-keyed state. A direct probe seeded an old agent/workspace, adopted an archive with no state sidecar, and retained both. Define whether missing metadata should reject or explicitly clear stale state before kit healing.

Prior unique-staging, permissions, exec-readiness and Bash empty-array fixes are present. The review worktree has no tracked edits. Live restore verification was not repeated; the author-reported live result is distinguished from local probe evidence above.

Comment thread acq.backends/msb.sh Outdated
Comment thread acq
Comment thread acq Outdated
@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Review feedback addressed in 465a66a.

  • Export and restore use MSB’s supported disk-only cold-recovery flow; every restore re-applies kits strictly.
  • Snapshot-based lifecycle commands now refuse clone-backed sandboxes rather than writing an archive that cannot recover the external clone workspace.
  • Snapshot kit state is authoritative, preventing extra kits from the restore environment from changing restored state.
  • Updated ADR/help/backend docs to match the implementation.

Verification:

  • PASS: PATH="/usr/bin:$PATH" ACQ_BATS_JOBS=1 ./scripts/test-acq-bats test/bats/70-msb-backend.bats test/bats/95-kit-ref-persistence.bats (44/44)
  • PASS: bash -n acq acq.backends/common.sh acq.backends/msb.sh test/bats/70-msb-backend.bats test/bats/95-kit-ref-persistence.bats
  • PASS: git diff --check
  • PASS: final adversarial review, no blockers.

Residual environment limits: full ./scripts/test-acq-bats exceeded the local execution timeout during its pre-existing cleanup hang; live MSB validation could not run because msb is unavailable. Markdown lint dependencies are also absent locally, so npm run lint could not start.

Please re-review.

@mogul
mogul force-pushed the feat/native-snapshot-restore branch from 465a66a to f4f4790 Compare October 22, 2026 08:17
@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Live validation correction in f4f4790

The previous update claiming default archive + --disk-only was supported was incorrect. Direct MSB 0.7.7 probing established:

  • Default disk archive + ordinary restore preserves durable guest state.
  • Full archive + ordinary restore preserves durable guest state in the mount-free probe but is not appropriate for acq’s external virtio-fs topology.
  • --disk-only is supported only for installed full snapshot references, not direct .msb archives.

The implementation therefore uses the verified archive pairing: msb snapshot create --guest-flush auto -o <archive> followed by ordinary msb restore <archive>. The added scripts/probe-msb-snapshot-restore records these compatibility cases on an MSB-capable host.

The valid safety remediations from review remain:

  • Snapshot/recreate/rm --snapshot reject clone-backed sandboxes before any destructive action.
  • Restore kit healing is strict for every persisted kit; ordinary resume healing retains its prior semantics.
  • Snapshot kit metadata is authoritative, including an empty extra-kit set.

Verification:

  • PASS: focused Bats, 44/44.
  • PASS: Bash syntax, ShellCheck for the new probe, git diff --check, and executable-bit check.
  • PASS: direct MSB 0.7.7 probe: default archive ordinary restore, full archive ordinary restore, local full ordinary restore, and local full disk-only restore preserved the sentinel; only archive --disk-only is rejected as documented.
  • PASS: live VERIFY_KEEP=1 ./scripts/verify-native-snapshot-restore: named restore, explicit archive copy restore, guest sentinel preservation, SSH-agent vsock forwarding, and acq recreate.
  • PASS: final adversarial review found no blockers.

Please re-review.

@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the current main conflicts in ff58ae5 and pushed the merge.

The resolution preserves main's generic OpenCode and provider-facts behavior alongside the PR's verified MSB disk-snapshot recovery. It also fixes a merge-visible restore gap: restore now loads provider facts before deriving secret bindings and passes the current --secret ENV@HOST flags to MSB. A regression verifies a kit-provided credential variable and endpoint are used.

Verification:

  • PASS: focused conflict-related Bats suites, 102/102.
  • PASS: Bash syntax, git diff --check, and executable-bit check.
  • PASS: final adversarial review, no blockers.

Known gap: full offline Bats was not rerun because it has previously exceeded the local execution limit; live MSB verification was not rerun after this merge because it requires the configured host environment. The pre-merge live snapshot verifier passed named restore, explicit archive restore, SSH-agent forwarding, and recreate.

@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Completed the local rebase onto current main (20d6bab, including the v1.16.1 patterns-kit pin) and force-pushed the rewritten stack.

Conflict resolution retained the snapshot lifecycle behavior and mainline generic OpenCode/provider-facts updates. Follow-up local review found and corrected three restore-path integration issues introduced or exposed by the rebase:

  • Pass current provider-derived --secret ENV@HOST bindings to MSB restore.
  • Clear stale name-scoped restore/config/provenance state before adopting archive sidecars.
  • Recreate and read-only mount the destination trusted host-config directory during restore, preserving the ADR-0035 readonly kit-file boundary.

Verification:

  • PASS: direct MSB Bats suite, 39/39.
  • PASS: Bash syntax, ShellCheck, git diff --check, and executable-bit check.
  • PASS: targeted adversarial review, no blockers.

The full suite was intentionally left to CI, which runs on this push. Residual live gap: the new fresh-host-config read-only restore binding is offline-asserted but not re-run live in this environment.

@mogul
mogul force-pushed the feat/native-snapshot-restore branch from ff58ae5 to a8b5e68 Compare October 22, 2026 09:30
@mogul
mogul force-pushed the feat/native-snapshot-restore branch from a8b5e68 to dc1d3f8 Compare October 22, 2026 09:31
@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased locally again onto current main at efc08ee (v4.1.0 release plus merge-queue CI update) and force-pushed the base-aligned stack. The recurring SBX test-stub conflict was resolved locally with the same reviewed snapshot|restore unsupported behavior.

Fast post-rebase integrity checks passed: Bash syntax, git diff --check, executable-bit validation, and conflict-marker scan. The focused MSB suite and targeted review passed before this release/CI-only rebase; CI is running the complete checks on this push.

@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 — dc1d3f8

The earlier help-text finding is resolved.

Major — acq.backends/msb.sh:1309-1313: Restore deletes the destination's config, provenance, kit record, and resource record before MSB accepts the archive, without checking whether the destination already exists. A rejected restore against an existing sandbox therefore leaves that sandbox running with its authoritative launch/workspace state deleted. Confirmed with the real adapter and an injected backend rejection: exit 17, original sandbox still present, metadata gone. Refuse existing destinations before cleanup and preserve prior state when restore fails; add failure-path coverage.

Live validation of the final host-config restore mount remains unverified; the reported live pass predates that change.

Assisted-by: OpenCode [GPT 6 Astra] skill:change-review

@mogul

mogul commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the latest restore-destination finding in 04666de, then fixed the Bash 3.2 CI portability failure in 0827249.

Restore now fails closed before staging or clearing anything when the destination exists, and also refuses to proceed when MSB cannot confirm the destination is absent. The new regressions verify both an existing destination and an indeterminate list probe preserve host config, workspace provenance, and restore-resource state without invoking msb restore.

The Bash 3.2 failure was unrelated to the restore change: GNU awk rejected the escaped quote regex used while parsing quoted agent-kit metadata, causing the generic SBX attach gate to reject a valid OpenCode kit. The parser now uses portable character comparisons.

Verification:

  • PASS: focused MSB restore tests, 41/41.
  • PASS: focused generic OpenCode migration-gate tests, 26/26.
  • PASS: Bash syntax, ShellCheck, git diff --check, and executable-bit validation.
  • PASS: all GitHub checks, including the complete 800-test Bash 3.2 offline suite, pre-commit, Markdown lint, link check, and PR title validation.

Please re-review.

@wz-gsa wz-gsa 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.

AI-assisted re-review (OpenCode) at0827249. Prior snapshot/clone/strict-extra-kit/existing-destination findings are resolved by the current implementation and focused probes. One new P2 regression remains in acq.backends/common.sh:860: literal single quotes inside the shell-single-quoted awk program are consumed by shell parsing. Valid agent metadata name: 'opencode' retains its quotes through the actual parser on Bash3.2, then fails the equality gate at:885. Double-quoted metadata passes. Use sprintf("%c",39) or correctly escaped shell quoting and cover both name and entrypoint single-quoted values. Syntax and ShellCheck alone do not detect it. All seven CI checks green; local focused run had three host-config path assertions expecting /var while actual canonical path was /private/var, plus one timeout-tool skip. Strict extra-kit injected failures now propagate; clone snapshot refuses before removal; existing/indeterminate restore destinations preserve metadata. Live final host-config mount remains unverified. Please fix the single-quote parser regression before merge.

@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 — 0827249fdeb0e4a47ba4a1b4051aee805c25d15d

The earlier help-text finding is resolved. The existing-destination guard added in 04666de preserves metadata when that destination is detected, but failure-path preservation remains incomplete.

Major — acq.backends/msb.sh:1327-1333,1378-1381: Restore still deletes name-keyed config, provenance, kit and resource records before MSB accepts the archive. With an absent destination carrying retained metadata (for example after recreate removes its source), an injected backend rejection returns 17 and leaves every record deleted. Preserve prior metadata until backend acceptance, or restore it on failure; add a backend-rejection regression, not only existing/indeterminate-destination tests.

Minor — acq.backends/common.sh:860: The single-quote comparisons sit inside Bash's single-quoted awk program, so Bash consumes those delimiters instead of passing literal apostrophes to awk. A valid kit declaring agent.name: 'opencode' and agent.entrypoint: 'opencode' now fails acq_validate_agent_builtin_kit_dir; identical plain and double-quoted controls pass. Compare with character code 39 (as the double-quote branch uses 34) and cover all three scalar forms.

Current-head GitHub checks report success. Live validation of the final host-config restore binding remains unverified; earlier live passes predate it.

Assisted-by: OpenCode [GPT 6.1 Sol] skill:change-review

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