Repository navigation
Conversation
|
Update from follow-up verification pass:
Verification:
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. |
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head Blocker: ADR-0030 is claimed twice, and the whole 0030/0031 window is contendedThis PR adds Audited every open quickstart PR, and it's tangled further than a single pair:
Worth noting this is the second instance of the same class in three days — #500 originally added a second Two things would help, in order:
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 |
|
Reviewed at 6289ba9. Adds acq-owned Intent: Matches the stated intent. The two findings below are gaps within that intent, not a different problem. Findings (2)
Below the bar: (a) Verdict: needs changes Assisted-by: OpenCode [claude-opus-5] team-code-review |
|
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 Re-checked all four just now, against
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 My read is unchanged on which should move: #504 keeps 0030, because its number is already cited from another repository — 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 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. |
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at 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
I transcribed the exact shape and ran it on both interpreters: Two details that make this narrower and more fixable than it looks:
Blocker 2 — the PR contains its own indictment
The
And I confirmed the cold-boot path at The fix is the same pair and the same three lines after Checked for sbx safety: 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
Full patch: +70/−2 across How I ran the tests, and why that mattersAll three pass under real bash 3.2.57 — but the invocation matters, because on my host a naive
Separately: two of this PR's own new tests are red on macOSThis is not pre-existing debt — I checked: The two failures are Cause: the All five workflows are Worth noting this is the same shape as blocker 1 — a real failure the gate structurally cannot observe. Method: static analysis at |
|
AI-assisted update pushed in Feedback Addressed
Verification |
basiliskus
left a comment
There was a problem hiding this comment.
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}.resourcesand passes itsvolumeentries directly tomsb 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. Inacq, snapshot helpers collide with new secret helpers andrmmust retain the new version guard. Inmsb.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. Indocs/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 mode100644; the documented./scripts/verify-native-snapshot-restorecommand returnsPermission denied. - Note — Prior Bash 3.2 blocker: the code now uses the guarded
${array[@]+...}expansion atacq.backends/msb.sh:3385-3386, andtest/bats/70-msb-backend.bats:91-97adds 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
bc94b02claims are present: full-state restore skips kit heal; disk-only heal failure propagates;.kitsis 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 checkBLOCKED (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
|
AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head I closed your residual risk. You noted:
This host is capable, so I ran the live cycle — eight sandboxes, two snapshots, all removed afterwards. 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 fixedBlocker 1 — ${_restore_volume_specs[@]+"${_restore_volume_specs[@]}"} || return 1Blocker 2, and your refinement beats my finding. I had said 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
|
b5b6efa to
bb388df
Compare
|
AI-assisted update: rebased onto current Blocking feedback addressed
Regression coverage added/updated
Verification 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
left a comment
There was a problem hiding this comment.
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:
- 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). - Resolved: the rebase removed the deleted
_acq_msb_grant_oci_devsseam while preserving main's secret helpers, version guard, merged kit environment behavior, and agent-kit documentation. - 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). - Resolved: restore now fails closed when exec readiness times out, and the probe preserves the real
msb execstatus (acq.backends/msb.sh:1237-1240,1266-1272). - Resolved:
scripts/verify-native-snapshot-restoreis mode 100755.
Two issues remain:
-
Major: make staged restore paths unique.
acq.backends/msb.sh:1056-1058maps identical archive bytes to the same${sha256}.msb, while each invocation unconditionally removes that shared path at1228-1236. Concurrent restores of the same archive can therefore overwrite or delete one another's input beforemsb restoreopens it. Keep a per-invocation private 0600 staging name and use the content digest only for metadata lookup; add a collision/concurrency regression test. -
Minor: correct the durable restore rationale.
docs/adr/0036-native-snapshot-restore.md:44-48still 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 atacq.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
|
AI-assisted update pushed in Feedback addressed
Verification Live msb snapshot/restore remains host-capability dependent and was not run in this Linux workspace. AI-generated changes require human review before deployment. |
bad0401 to
86b55d0
Compare
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>
86b55d0 to
7d18d4f
Compare
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
acq snapshot,acq restore,acq recreate, andacq rm --snapshot.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
./scripts/test-acq-bats test/bats/70-msb-backend.bats test/bats/117-msys-path-forms.bats(1..43).bash -n acq acq.backends/common.sh acq.backends/msb.sh acq.backends/sbx.sh scripts/verify-native-snapshot-restore.--severity=warning).git diff --check.740tests) andnpm run checkbefore stacking../scripts/test-acq-batsandnpm run checkexceeded 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:
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:
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.