fix(sbx): wait for startup barrier before attach

[mogul]

Summary

Fixes an sbx create-and-attach race where acq run opencode <path> could probe or attach before non-background kit startup commands finished writing agent config.

The sbx provision path now appends an acq-generated final startup kit, waits for its per-create completion marker after sbx create, and refuses to keep the sandbox if the marker is not observed. This keeps retries from bypassing the barrier through the existing-sandbox reattach path.

Fixes #506

Plan

• Add an sbx startup barrier kit generated by acq at create time.
• Wait for the per-create barrier token before provenance, postinstall probes, key validation, or attach.
• Remove the just-created sandbox if the barrier times out, so a later retry cannot silently attach to an unverified sandbox.
• Add offline bats regressions for the ordering and timeout/fail-closed behavior.
• Add a host-side live verifier for the slow startup config-write case.

Verification

Automated checks run in the sandbox:

bash -n acq acq.backends/sbx.sh scripts/test-acq-lib.sh scripts/verify-sbx-startup-barrier
PASS

shellcheck --severity=warning acq.backends/sbx.sh scripts/test-acq-lib.sh test/bats/90-sbx-startup-kit.bats scripts/verify-sbx-startup-barrier
PASS

./scripts/test-acq-bats test/bats/90-sbx-startup-kit.bats test/bats/30-dispatch-routing.bats test/bats/85-kit-provenance.bats test/bats/95-kit-ref-persistence.bats test/bats/116-clone-option.bats
PASS: 107/107

npm run lint
PASS: markdownlint-cli2, 0 issues

git diff --check
PASS

Full offline suite note:

./scripts/test-acq-bats
Ran clean through 463/519 tests before the 5-minute sandbox command timeout killed the run.
No failures were reported before timeout.

Secrets scan:

gitleaks detect --no-git -v
FOUND: existing .github/workflows/bash32-compat.yml fingerprint-like value BASH_SIGNING_KEY_FPR.
Assessment: not introduced by this PR; no new secret in this diff.

Adversarial review:

Ran a sub-agent adversarial diff review before commit.
Finding addressed: barrier timeout originally left the created sandbox behind, which could allow a later retry to bypass the barrier through the existing-sandbox path.
Fix added: timeout removes the unsafe sandbox and a bats regression covers this behavior.

Host-side live verification supplied by Bret:

./scripts/verify-sbx-startup-barrier
PASS: 5 passed, 0 failed
Observed slow startup config written before acq returned; config present immediately after create.

Rollback

Revert commit 7b0f156 to remove the generated startup barrier, tests, live verifier, and docs entry. Existing sandboxes are not migrated; the change only affects future sbx create provisioning through acq.

Security Impact

• Authentication/authorization: no direct changes.
• Secrets/data handling: no new secrets or secret storage; barrier marker contains only a per-create non-secret token.
• Attack surface: no new network access or dependencies.
• Safety behavior: fail-closed improvement. If startup completion cannot be verified, acq refuses to retain the sandbox for attach.

AI Attribution

AI-assisted implementation by OpenCode. Human host-side verification was supplied before PR creation.

[wz-gsa]

AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head 7b0f156, main at 30f9ecf. I filed #506, so I reviewed this against the root cause I reported rather than against the PR description.

The mechanism is right, and it closes the race I reported. Docker's kit-reference is explicit that non-background startup commands are sequenced relative to each other but do not gate the entrypoint. A final acq-generated startup kit therefore runs after every earlier non-background command, so its marker really is a completion signal for them — and acq polling for that marker before ensure_valid_key → ensure_opencode_postinstall → acq_backend_attach closes the window. The --kit ordering holds: user and CLI kits go through KITS → _acq_sbx_kit_flags (sbx.sh:531) and the barrier is appended after (:537), so nothing user-supplied can land behind it.

Three findings, none of which contradict that.

Should-fix: the re-attach path is not covered, and the PR says it is

The description says the barrier "keeps retries from bypassing the barrier through the existing-sandbox reattach path." It does that only for the specific case where the barrier itself timed out — via the acq_backend_terminate on sbx.sh:553. It does not cover the path generally: _acq_sbx_wait_for_startup_barrier has exactly one call site (:548, inside acq_backend_provision).

acq:1068's re-attach-by-name branch calls acq_backend_ensure_kits_applied → sbx kit add, which re-runs kit startup commands, then attaches with no barrier wait. That is the same class of race as #506, on the path a resumed sandbox actually takes. It is arguably out of scope for this fix — the sbx ≥ 0.38 recreate restriction means kit add often can't extend a live sandbox anyway — but the claim as written is broader than the code. Either narrow the sentence to the timeout case, or add the wait after a successful heal.

Should-fix: nothing offline proves the token is compared

scripts/test-acq-lib.sh:134 answers ready for any snippet containing startup-complete, without inspecting the token:

*"startup-complete"*) printf 'ready\n' ;;

Correct for unrelated tests — they shouldn't have to know about the barrier. But it means the token comparison in the guest probe (sbx.sh:389) is never exercised: a build that dropped the = '$token' clause entirely, or compared the wrong variable, would pass the full offline suite. The ordering test asserts assert_regex "$log" 'startup-barrier', which matches the kit name, not the token.

Cheap fix: one test with a stub that echoes ready only when the snippet contains the token it was told to expect, and a negative case where a stale token is present and the wait times out. That is the difference between "we wait for a marker" and "we wait for this create's marker" — which is the whole point of making it per-create.

Nit: the token is not unique within a second

sbx.sh:536 derives it as acq-$$-$(date +%s). Measured: three successive derivations in one shell produce an identical token, because $$ is the shell's PID and the epoch has 1-second resolution. Two acq invocations get different PIDs so a live collision needs PID reuse in the same second — unlikely, not impossible on a busy host, and PID reuse is exactly the kind of thing that bites once a year and is undebuggable.

More importantly the marker path is fixed (/tmp/acq/startup-complete) and /tmp is shared with every kit and the agent. A stale file from a previous create in the same second would read as ready. date +%s plus a few bytes of randomness — $RANDOM, or od -An -tx1 -N4 /dev/urandom — removes the whole question for one line.

Also, the probe interpolates the token into a sh -c string single-quoted inside double quotes. PID+epoch can never contain a quote, so this is not exploitable today — but if the token ever gains entropy from a less constrained source, that construction breaks. Worth a case guard on the token charset, or keeping the generator provably alphanumeric.

Verified as holding

• Fail-closed on timeout is real. sbx.sh:548-556 returns 1, removes the sandbox, and emits an actionable message when removal itself fails. I transcribed the exact shape (acq_backend_terminate … || echo warn; return 1) and confirmed under set -euo pipefail that the warning is printed and the function still returns 1 — the || doesn't swallow the failure signal.
• TIMEOUT=0 is a one-shot probe, not a zero-shot. The probe runs before the deadline check, so the timeout test exercises the real code path rather than short-circuiting it. Confirmed by transcribing the loop and counting probes via a marker file: exactly 1.
• The || out="" added to _acq_sbx_wait_for_exec_ready (:375) is correct, though defensive rather than load-bearing here: acq runs set -euo pipefail, so a failing sbx exec makes the pipeline rc nonzero and a bare assignment would abort. I verified the abort happens on a direct call (script rc=7) and not when the function is invoked under if ! — which is how both waiters are called (:605, :548), so errexit is suppressed at those sites either way. Keeping the guard is right; it just isn't what's protecting you today.
• The ordering test is genuinely discriminating: it captures line numbers from the stub call log and asserts create < barrier < opencode --version < sbx run, so a barrier moved after the postinstall probe fails it. That's the right assertion for this fix.
• The timeout test asserts sbx rm --force timeoutbox in the call log and [ -f "$STUBDIR/.removed" ] — a positive observable, not just absence.
• scripts/verify-sbx-startup-barrier reproduces my original probe shape (4-second sleep then a config write) and greps for STARTUP_CONFIG_WRITTEN and CONFIG_PRESENT=yes after acq create returns. That's the live evidence the offline suite can't give.
• bin/injection-scan.sh clean. No new egress, no new secret handling; the marker is a non-secret token, as the PR states.

---

Method: static analysis at 7b0f156 plus transcription-and-run of the barrier wait loop, the timeout/terminate shape, the token derivation, and the set -e/pipefail assignment semantics. No PR code executed, no sandbox created. UNVERIFIED: the live behaviour — I did not run scripts/verify-sbx-startup-barrier (it needs a sandbox-capable host with create-time secrets), so the "5 passed, 0 failed" in the PR body is the author's evidence, not mine.

Thanks for picking this up quickly — the fix addresses the reported cause rather than the symptom, which is the part I care about most.