Repository navigation
feat(acq-kits): openchamber wrapper owns a shared opencode server - #234
Conversation
Redesign the openchamber kit so the browser UI and an optional terminal TUI share one live OpenCode session by default, and simplify what the kit's startup manages. - Ship an `opencode` wrapper at ~/.local/bin/opencode (first on the base-image PATH, so it shadows the real binary and is what the sandbox entrypoint runs). With no args it idempotently starts a shared `opencode serve` on 0.0.0.0:4096, prints host-connect instructions, and offers to attach a TUI; with args it passes through to the real opencode. - Reduce the startup script to managing OpenChamber only (install on first boot + respawn supervisor in skip-start mode); it no longer starts or supervises `opencode serve` — the wrapper owns that, on demand. - Publish both container ports 3000 (OpenChamber) and 4096 (shared server); run the shared server unsecured (the sandbox is the security boundary). - Rework scripts/verify: three-state offline gate (SKIP when python deps missing, real error on failure), a preferred RUN_ACQ live path (creates via acq, publishes 3000/4096, exercises the wrapper + on-demand server), and a gated RUN_SBX path that requires a pre-translated sbx-v2 kit. - Drop the "*.npmjs.org" allow host: registry.npmjs.org serves metadata and tarballs, and a leading-* host breaks acq's (unquoted) neutral->sbx translation. Documented in-spec. - Add docs/decisions/wrapper-entrypoint-owns-server.md and annotate the superseded premise of openchamber-mixin-not-sandbox-kit.md. Update README, TROUBLESHOOTING, and kits.yaml. Ports need a manual `acq ports --publish` under acq until the upstream publishedPorts translation (quickstart#221) reaches the pinned kit ref; tracked as #233. Co-authored-by: OpenCode Agent <[email protected]>
…rver) The openchamber `opencode` wrapper is the sandbox entrypoint (it shadows the real binary via PATH). Its no-arg path backgrounded `opencode serve` with `nohup … &` and then returned (`exit 0`), so PID 1 exited and the sandbox transitioned running -> stopped moments after `acq run` — visible as a sandbox that "stops on its own" when you decline the TUI prompt or run non-interactively. Introduced in this branch's wrapper redesign. A Docker sandbox is "running" only while its entrypoint / PID 1 is alive. Fix the wrapper's no-TUI path to hold a foreground process as PID 1: - if it started the server here, `wait` on that background job; - if a server already owns :4096, park on a liveness loop and only `exec` a fresh foreground server if the existing one disappears (avoids a double-bind that, under `set -e`, would itself kill the entrypoint). The interactive "yes -> opencode attach" path already `exec`ed, so it was unaffected. - files/.local/bin/opencode: foreground handoff + "must stay PID 1" note. - spec.yaml, README, docs/decisions/wrapper-entrypoint-owns-server.md: describe the foreground handoff and record the regression it prevents. - TROUBLESHOOTING.md: add "sandbox stops on its own right after acq run"; update the `acq exec … opencode` recipes to start the server detached (the wrapper now foregrounds). - scripts/verify: add step 9 entrypoint-liveness regression guard (assert the no-arg wrapper blocks rather than returning); start the wrapper detached in step 5 so the live check doesn't hang. Verified: host-side `RUN_ACQ=1 scripts/verify` passes all checks (including the new liveness guard) on a sandbox-capable host. Co-authored-by: OpenCode Agent <[email protected]>
…is quit The prior fix kept the sandbox running on the declined/non-interactive path, but the "yes, attach a TUI" path still `exec`ed `opencode attach`, making the TUI itself PID 1. Quitting the TUI then ended the entrypoint and stopped the sandbox (and the OpenChamber browser UI with it) — contradicting the kit's premise that the browser and TUI share one long-lived server. Run the attached TUI as a CHILD (not exec) with `|| true`, so quitting it returns to the wrapper. Extract the PID-1-hold into a hold_pid1() function that every no-arg path ends on (decline, non-interactive, and attach-then-quit), so the sandbox and the shared server survive a TUI exit. - files/.local/bin/opencode: TUI as child + hold_pid1() shared tail. - spec.yaml, README, docs/decisions/wrapper-entrypoint-owns-server.md: document TUI-exit survival; ADR records both fixed variants (return, exec). - TROUBLESHOOTING.md: add "sandbox stops when I quit the attached TUI". - scripts/verify: add step 10 guard asserting the attach line is not exec'd and the wrapper ends in a hold_pid1 call. Verified: host-side `RUN_ACQ=1 scripts/verify` passes all checks, including the new step 10 TUI-exit-survival guard. Co-authored-by: OpenCode Agent <[email protected]>
markdownlint MD028/no-blanks-blockquote: a bare blank line separated two adjacent blockquotes in the openchamber README. Join them with a `>`-only line so they read as one blockquote and the Markdown Lint CI check passes. Co-authored-by: OpenCode Agent <[email protected]>
…mber docs The openchamber README/TROUBLESHOOTING are the acq-facing docs, but a few runnable examples still invoked the raw `sbx` backend CLI. Since acq passes unintercepted commands and extra args straight through to the backend, the acq form is equivalent and consistent: - `sbx ports … [--publish …]` -> `acq ports …` - `sbx stop … && sbx run …` -> `acq stop … && acq run …` - `sbx policy log …` -> `acq policy log …` (passthrough) - drop the parenthetical "(sbx ports …)" hints next to `acq ports`. Left unchanged (correctly): references to the `sbx` *backend*, the sbx-v2 *schema*, `backend_extras.sbx.*` spec fields, the historical `sbx-kits/` path, and the RUN_SBX advanced/legacy path — these name the backend or internals, not commands a user runs. Co-authored-by: OpenCode Agent <[email protected]>
There was a problem hiding this comment.
Request changes — the design is genuinely good, but there's a defect that trips the exact failure this whole file exists to prevent, so it shouldn't land as-is.
The concept is clever and the design comments are excellent. I verified the scary parts and most are solid:
- Self-recursion: PASS — canonical self-path compare (not argv[0]), skips its own dir through symlink/dup PATH spellings, exits 127 cleanly if no real opencode exists. No fork-bomb.
- Passthrough: PASS —
exec "$real_opencode" "$@", args preserved (no re-split/re-glob). set -euon the PID-1 path: PASS — this was my top worry and it's clean; every startup hazard (health-probe non-zero,readat EOF,caseno-match) is in a condition context or|| true'd, so errexit doesn't kill startup before the hold.- Non-interactive TUI: PASS —
[ -t 0 ] && [ -t 1 ]gates the prompt; no TTY falls straight through to the hold, and theread || _ans=""defaults to decline.
The bug (Medium, must-fix): the started_here=1 hold uses a bare wait with no liveness re-check (~line 172). If the server it launched dies (crash, OOM, or the race below), wait returns, hold_pid1 returns, and — since that's the last line — PID 1 exits → the sandbox goes running→stopped, the precise thing this wrapper is built to prevent. The other branch (~line 182) does it right: it polls serve_up every 30s and re-takes the foreground. The two paths are asymmetric. Make started_here self-heal like the else branch (while serve_up; do wait … || sleep 5; done then exec … serve).
Contributing race (Medium): serve bring-up is a TOCTOU — detection is a curl /health only, no lock/pidfile, so two near-simultaneous no-arg starts both nohup … serve and race the :4096 bind; the loser dies and its owner's bare wait returns → same PID-1 exit. A lock/pidfile around the launch closes it.
Lower severity: /tmp/opencode-serve.log is a predictable path (low, sandbox-bounded — worth an mktemp or a fixed sandbox-local dir). scripts/verify:281's ~ is only in the failure-message string, not the test (the actual test -x ~/.local/bin/opencode runs inside sh -c in-sandbox where ~ expands), so it's cosmetic. spec.yaml is a valid hybrid/v1 mixin and drops the wrapper 0755. Nice.
Honesty note on the loopback-only claim: the "ports are host-loopback only" guarantee depends on the adapter publishing loopback-scoped — and spec.yaml itself notes acq's translator doesn't yet carry publishedPorts (needs a manual acq ports --publish). GSA-TTS/agentic-coding-quickstart#221 is what adds that carry. So the 0.0.0.0:4096 bind is only safe once GSA-TTS/agentic-coding-quickstart#221 lands and the operator publishes loopback; worth stating that dependency in the security note.
CI: Markdown Lint is red on the recurring npm-audit js-yaml gate (patterns audits at moderate, this branch predates the fix). I'll push the same override bump to your branch to clear it — unrelated to your code.
Asks: (1) self-heal the started_here hold; (2) lock/pidfile around serve launch; (3) a verify regression test for "serve dies → PID 1 must not exit" (current tests check blocking + not-exec, but not this); (4) note the GSA-TTS/agentic-coding-quickstart#221 publishedPorts dependency in the security section. The wrapper's bones are right — this is hardening the failure path, not a redesign.
Markdown Lint CI gates on npm audit --audit-level=moderate; this branch predated the fix and was red on js-yaml GHSA-52cp-r559-cp3m + linkify-it GHSA-v245-v573-v5vm. Bump the linter overrides (js-yaml 4.2.0->4.3.0, +linkify-it 5.0.2) and regenerate the lockfile. 0 vulnerabilities at moderate. Co-authored-by: OpenCode Agent <[email protected]>
Address the review must-fixes on the PID-1 hold. All are failure-path hardening, not a redesign. 1. Self-heal the hold (Medium, must-fix). The old `started_here` branch did a bare `wait`; if the server it launched died (crash/OOM/lost race), `wait` returned, hold_pid1 returned, and PID 1 exited -> sandbox stopped — the exact bug this wrapper prevents. Replace the asymmetric two-branch hold with ONE self-healing loop: poll the server and re-launch it if it ever stops answering, so a dead server can never let the loop return. 2. Close the serve-launch TOCTOU (Medium). Bring-up detected only via `curl /health`, so two near-simultaneous no-arg starts both `nohup … serve` and raced the :4096 bind (loser dies). Add an atomic mkdir lock (serve_launch_once) with a PID file and stale-lock reclaim (dead holder -> lock reclaimed), so exactly one wrapper launches. 3. Move the serve log off a predictable /tmp path (Low). Now ~/.local/state/openchamber/opencode-serve.log in a 0700 dir; docs/verify updated to read it there. 4. Security note: state the loopback-only guarantee depends on the adapter publishing loopback-scoped, which is acq's publishedPorts translation (quickstart#221). Also update publishedPorts docs now that quickstart#221 is MERGED: acq carries backend_extras.sbx.publishedPorts, so ports 3000/4096 publish at create time with no manual step (older acq still gets the documented fallback). verify's step 2b now re-publishes idempotently (works on old and new acq). scripts/verify: add step 11 — a live self-heal regression guard that brings the server up, KILLs it, and asserts a server answers again (hold_pid1 re-launched; PID 1 held). Verified: shellcheck-clean parses (sh -n / bash -n); offline simulations of the lock (8 racers -> 1 launch), stale-lock reclaim, and the self-heal loop (server death -> reheal, loop never returns); markdownlint 0 errors. Co-authored-by: OpenCode Agent <[email protected]>
|
Thanks for the thorough review — especially verifying the scary parts. All four asks are addressed in 6cc4700, plus the two lower-severity notes. Host-side (1) Self-heal the hold (must-fix). You were right that the (2) Serve-launch TOCTOU. Added (3) verify regression test. New step 11: brings the server up, (4) loopback-only honesty. The security note now states the guarantee depends on the adapter publishing loopback-scoped, i.e. the Low-sev: serve log moved off the predictable Verification: host-side |
|
I'm taking time to handle (part of) #233 and then will ask for a re-review. |
…t step (#233 Increment A) quickstart#221 is merged: acq's neutral→sbx translator now carries backend_extras.sbx.publishedPorts, so applying the openchamber kit publishes container ports 3000/4096 to the host at create time with no manual `acq ports --publish`. Adopt that here (Increment A of #233): - scripts/verify step 2b: instead of always publishing manually, VERIFY the ports were auto-published at create; only fall back to an explicit publish on an older acq that predates #221 (emitted as a non-fatal WARN, not a FAIL). Add a warn() helper for that non-fatal path. - kits.yaml + README: replace the "translator does not yet carry publishedPorts; publish manually" wording with "carried at create (quickstart#221); older acq falls back" and reframe the remaining gap as msb port/background plumbing. Increment B (msb/ppp parity) stays blocked and #233 stays open: acq still has no neutral port-publish/background vocabulary and the msb adapter consumes neither backend_extras.msb ports nor a background flag. No behavior change to the wrapper or startup script. Co-authored-by: OpenCode Agent <[email protected]>
|
Follow-up: adopted the now-merged GSA-TTS/agentic-coding-quickstart#221 publishedPorts translation — #233 Increment A (6bda74b).
Host-side #233 Increment B (msb/ppp parity) stays blocked and the issue stays open — acq still has no neutral port-publish/background vocabulary and the msb adapter consumes neither. Status recorded on the issue. |
|
Clarification on the step 2b verification (my previous "no WARN" note was conditional, not unconditional):
So the create-time behavior is verified on a current acq; the WARN is just the guard correctly detecting an older acq and degrading gracefully. Keeping the WARN non-fatal by design. |
|
@wz-gsa, re-review? |
wz-gsa
left a comment
There was a problem hiding this comment.
Re-review — both findings fixed, and fixed well. Approving.
W3(d) asymmetric hold (the PID-1-exits bug): hold_pid1 is now a single self-healing loop for every path — serve_up → park 5s → re-check, and relaunch if the server dies. The bare wait that could let PID 1 return (and stop the sandbox) is gone. Correct call on polling rather than wait, too, since the wrapper may not own the server it's holding (pre-existing or from a racing wrapper).
W4(a) TOCTOU serve-launch race: now guarded by an atomic mkdir lock (SERVE_LOCK) with stale-lock reclaim (records holder PID, reclaims if the holder is gone) so a killed holder can't wedge every future launch into the wait-only branch. Solid.
Bonus: SERVE_LOG moved to a sandbox-local state dir with an mktemp fallback (my low-sev /tmp nit), and — the part I most wanted — scripts/verify now has regression guards for the exact failure modes: "wrapper no-arg path RETURNED → PID 1 exits" and "sandbox survives an attached TUI being quit." That's the coverage that keeps this from silently regressing.
The loopback-only-ports dependency I flagged is now satisfied — #221 (publishedPorts carry) has merged. CI all green. Good work on the hardening.
Redesigns the opt-in
openchamberacq kit so the browser UI and an optionalterminal TUI share one live OpenCode session by default, driven by an
opencodewrapper that owns a single on-demand shared server. The kit's startup is reduced
to managing OpenChamber only.
Under acq, ports need a one-time
acq ports --publishuntil the upstreamtranslator fix (GSA-TTS/agentic-coding-quickstart#221) reaches the pinned kit
ref — tracked in #233.
Testing
validate-kits.py --strictandscripts/validate_repo.py: passsh -n(wrapper + startup),bash -n(verify)idempotent re-run, interactive yes→attach / no→recipe, non-interactive exit
RUN_ACQ=1verify control flow exercised against a mockedacq(13/13)Live end-to-end on a real host/
acqwas not run here (no sbx/acq in thisenvironment). Reviewer should run
RUN_ACQ=1 ./scripts/verifyfrom the kit dir.AI assistance
Authored with OpenCode agent assistance; disclosed per policy.
Relates to #233.