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. |
7d18d4f to
86dd0e6
Compare
4859865 to
f2bf6e9
Compare
basiliskus
left a comment
There was a problem hiding this comment.
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
f2bf6e9 to
20579dc
Compare
c74b7e0 to
d7ab52c
Compare
|
@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
left a comment
There was a problem hiding this comment.
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
… 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>
edb18da to
f574921
Compare
Review response —
|
wz-gsa
left a comment
There was a problem hiding this comment.
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.
|
Review feedback addressed in 465a66a.
Verification:
Residual environment limits: full Please re-review. |
465a66a to
f4f4790
Compare
|
Live validation correction in f4f4790 The previous update claiming default archive +
The implementation therefore uses the verified archive pairing: The valid safety remediations from review remain:
Verification:
Please re-review. |
|
Resolved the current The resolution preserves Verification:
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. |
|
Completed the local rebase onto current 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:
Verification:
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. |
ff58ae5 to
a8b5e68
Compare
a8b5e68 to
dc1d3f8
Compare
|
Rebased locally again onto current Fast post-rebase integrity checks passed: Bash syntax, |
basiliskus
left a comment
There was a problem hiding this comment.
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
|
Addressed the latest restore-destination finding in 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 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:
Please re-review. |
wz-gsa
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
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
acq snapshot,acq restore,acq recreate, andacq rm --snapshot.acq rm --snapshotan export followed by permanent cleanup of derived volumes and live host metadata; digest-keyed snapshot sidecars remain available for restore./var->/private/varprovenance-mount failures.Rebase
Rebased onto
mainafter #584 merged. The six previously inherited generic-OpenCode commits were intentionally omitted because their squash merge is already present inmain; this resolves the GitHub merge conflict without dropping snapshot functionality.Verification
89tests).npm run check, including gitleaks, Markdown lint, ShellCheck, complete offline Bats, and executable-bit validation.git diff --check.edb18daare green, including pre-commit and the bash 3.2 offline suite.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 completedacq recreate.--fullexported snapshot plus--disk-onlyfailed because the installed MSB requires an internal snapshot reference not expressible bymsb restore. The implementation now uses MSB’s supported default disk snapshot format and ordinary cold restore.Retained Verification Sandboxes
The live verifier retained
snaptest-opencodeandsnaptest-opencode-copyfor 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.