Skip to content

feat(acq-kits): openchamber wrapper owns a shared opencode server - #234

Merged
mogul merged 9 commits into
mainfrom
feat/openchamber-wrapper-owns-server
Jul 23, 2026
Merged

mogul merged 9 commits into
mainfrom
feat/openchamber-wrapper-owns-server

Conversation

@mogul

@mogul mogul commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Redesigns the opt-in openchamber acq kit so the browser UI and an optional
terminal TUI share one live OpenCode session by default, driven by an opencode
wrapper 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 --publish until the upstream
translator fix (GSA-TTS/agentic-coding-quickstart#221) reaches the pinned kit
ref — tracked in #233.

Testing

  • validate-kits.py --strict and scripts/validate_repo.py: pass
  • sh -n (wrapper + startup), bash -n (verify)
  • Wrapper exercised against a stubbed opencode: passthrough, no-arg serve,
    idempotent re-run, interactive yes→attach / no→recipe, non-interactive exit
  • RUN_ACQ=1 verify control flow exercised against a mocked acq (13/13)

Live end-to-end on a real host/acq was not run here (no sbx/acq in this
environment). Reviewer should run RUN_ACQ=1 ./scripts/verify from the kit dir.

AI assistance

Authored with OpenCode agent assistance; disclosed per policy.

Relates to #233.

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]>
@mogul
mogul requested a review from a team as a code owner July 23, 2026 07:52
@mogul
mogul marked this pull request as draft July 23, 2026 09:06
mogul added 2 commits July 23, 2026 02:33
…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]>
@mogul
mogul marked this pull request as ready for review July 23, 2026 09:51
mogul added 2 commits July 23, 2026 02:54
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]>

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

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 -eu on the PID-1 path: PASS — this was my top worry and it's clean; every startup hazard (health-probe non-zero, read at EOF, case no-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 the read || _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.

wz-gsa and others added 2 commits July 23, 2026 11:38
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]>
@mogul

mogul commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

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 RUN_ACQ=1 scripts/verify passes all checks (including the new self-heal guard).

(1) Self-heal the hold (must-fix). You were right that the started_here bare wait was the defect — a dead server would return from wait, hold_pid1 would return, and PID 1 would exit. Removed the asymmetric two-branch hold entirely and replaced it with a single self-healing loop: poll the server, re-launch it if it ever stops answering, never return. So a crash/OOM/lost-race server death now re-launches instead of stopping the sandbox.

(2) Serve-launch TOCTOU. Added serve_launch_once() guarded by an atomic mkdir lock + PID file: exactly one racing wrapper launches, the rest wait for the port. Includes stale-lock reclaim (dead holder PID → lock reclaimed) so a killed launcher can't wedge future starts.

(3) verify regression test. New step 11: brings the server up, pkills the opencode serve process, and asserts a server answers again — i.e. hold_pid1 re-launched and PID 1 held. (Existing steps 9/10 still cover blocking + not-exec.)

(4) loopback-only honesty. The security note now states the guarantee depends on the adapter publishing loopback-scoped, i.e. the publishedPorts translation. Since GSA-TTS/agentic-coding-quickstart#221 has now merged, I also flipped the ports docs across README/TROUBLESHOOTING/spec/ADR from "manual step required" to "published at create time (#221), with an older-acq fallback documented," and made verify's port-publish step idempotent so it works on both old and new acq.

Low-sev: serve log moved off the predictable /tmp path to ~/.local/state/openchamber/opencode-serve.log (0700 dir). Agreed the verify:281 ~ was cosmetic (the test -x runs in-sandbox) — left as-is.

Verification: host-side RUN_ACQ=1 scripts/verify green end-to-end; plus offline sims of the lock (8 racers → 1 launch), stale-lock reclaim, and the self-heal loop (server death → reheal, loop never returns); markdownlint clean. Thanks again — this hardened the failure path exactly where it needed it.

@mogul

mogul commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor Author

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]>
@mogul

mogul commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: adopted the now-merged GSA-TTS/agentic-coding-quickstart#221 publishedPorts translation — #233 Increment A (6bda74b).

Host-side RUN_ACQ=1 scripts/verify passes end-to-end, confirming ports are published at create with no manual step (step 2b PASS, no WARN, on a current acq).

#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.

@mogul

mogul commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

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.

@mogul

mogul commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@wz-gsa, re-review?

@mogul
mogul requested a review from wz-gsa July 23, 2026 17:25

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

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.

@mogul
mogul merged commit 0e04073 into main Jul 23, 2026
7 checks passed
@mogul
mogul deleted the feat/openchamber-wrapper-owns-server branch July 23, 2026 22:49
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.

2 participants