Skip to content

docs(adr): PTY Mode — remote sandboxed terminal sessions - #1477

Closed
chaodu-obk[bot] wants to merge 2 commits into
mainfrom
adr/pty-mode
Closed

docs(adr): PTY Mode — remote sandboxed terminal sessions#1477
chaodu-obk[bot] wants to merge 2 commits into
mainfrom
adr/pty-mode

Conversation

@chaodu-obk

@chaodu-obk chaodu-obk Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Proposes an opt-in PTY mode as a second session backend alongside the existing ACP session pool, based on the Discord discussion comparing OAB with OpenDray and Herdr.

Review Contract

Goal

Record an ADR (Status: Proposed) that captures the design decision space for a PTY-based remote terminal capability: the user need, the architecture options, the security trade-offs, and a prior-art survey of OpenDray, Herdr, and Claude Code cross-session messaging mapped to implementation phases.

Non-goals

  • No code changes: this PR adds a single ADR document only.
  • Does not commit the project to implementation: the ADR is Proposed, not Accepted; round-2 review identified blockers (positioning vs DESIGN.md Thin Bridge, same-pod blast radius, auth model, scrollback defaults, pool-reuse claims) that must be resolved or the decision reshaped before any implementation ADR is accepted.
  • Does not decide the delivery form (in-process backend vs separate openab-pty runtime vs ACP observability extension) — that decision is pending maintainer direction based on review feedback.

Accepted Residual Risks

  • The ADR as written asserts "sandbox posture unchanged" and session-pool reuse claims that review found overstated; these remain in the document until the maintainer chooses a revision direction (revise in place, trim to Phase-1 experiment, split to a separate runtime ADR, or close in favor of ACP observability). Mitigation: the review verdict (Changes Requested) and the OpenAB PR Review status gate block acceptance of the ADR as-is.
  • Prior-art references cite external file paths and library names that may rot as upstreams evolve; acceptable for a survey section in a Proposed ADR.

Acceptance Criteria

  • docs/adr/pty-mode.md follows the established ADR format (header block, numbered sections, alternatives with rationale).
  • Prior-art claims in section 6 are attributable to public sources (OpenDray source, Herdr docs, Claude Code docs).
  • Maintainer has chosen a direction for the identified review blockers (revise / trim / split / close) before the ADR moves from Proposed to Accepted.

Follow-ups

  • Address review blockers per the chosen direction: reconcile positioning with DESIGN.md, specify the isolation boundary, strengthen the auth/token model, resolve the scrollback secrets contradiction, and align section 5 phase bullets with section 6 adopt-in mappings.
  • Potential follow-up ADRs: openab-pty as a separately deployable runtime; ACP observability extension (Alternative A); JSONL transcript as a structured side channel.

Architecture

flowchart LR
    subgraph clients_acp["Persona 1 — Agent Broker (existing)"]
        discord["Discord / Slack / Telegram<br/>turn-based messages"]
    end

    subgraph clients_pty["Persona 2 — Terminal Server (new)"]
        web["Web terminal<br/>(xterm.js)"]
        mobile["Mobile / desktop app<br/>(terminal widget)"]
    end

    subgraph pod["OpenAB Unified Binary — K8s pod (sandbox unchanged)"]
        adapters["Platform adapters"]
        ws["GET /pty/{session}<br/>WebSocket + Bearer token"]
        pool["Session pool<br/>(lifecycle · TTL · eviction · pgid kill)"]
        acp_backend["ACP backend<br/>stdio JSON-RPC"]
        pty_backend["PTY backend (new)<br/>portable-pty + scrollback ring buffer"]
        agent_acp["Agent subprocess<br/>(structured ACP events)"]
        cli_pty["CLI subprocess<br/>bash / claude / codex / kiro"]
    end

    discord --> adapters --> pool
    web -- "raw PTY bytes (binary frames)<br/>resize / detach (text frames)" --> ws
    mobile --> ws
    ws --> pool
    pool --> acp_backend --> agent_acp
    pool --> pty_backend --> cli_pty
    pty_backend -. "Phase 4: idle/blocked notification" .-> adapters

    style pty_backend fill:#1a7f37,color:#fff
    style ws fill:#1a7f37,color:#fff
    style cli_pty fill:#1a7f37,color:#fff
Loading

Key points:

  • Dual persona: existing "Agent Broker" (ACP, turn-based, Discord/Slack) + new "Terminal Server" (PTY, real-time keyboard, WebSocket terminal clients)
  • Fills an empty quadrant: remote + sandboxed + full raw-terminal visibility — PTY subprocess stays inside the pod, unlike OpenDray (host-resident) and Herdr (laptop-local)
  • Session pool reuse: PTY is a new session backend, not a parallel system — lifecycle, eviction, and pgid kill handles carry over
  • Security posture unchanged: sandbox mandatory, Bearer token on WebSocket upgrade, feature-gated and disabled by default; token possession documented as equivalent to pod shell access
  • Phased plan: core PTY backend → xterm.js web client → lifecycle hardening → optional messaging-platform notification bridge

Prior art

§6 of the ADR maps concrete techniques from three adjacent projects to implementation phases:

  • OpenDray — ring-buffer reconnect cursor, terminal-capability response filtering, pure lifecycle state machine, server-side virtual terminal, idle-notification pipeline
  • Herdr — semantic agent state detection (working/blocked/idle/done), race-safe waits, layered restore taxonomy, secrets-safe scrollback default
  • Claude Code cross-session messaging — per-session UDS inbox with filesystem discovery, plain-text message contract, permission-class trust model, loop throttling

Notes

  • ADR only — no code changes
  • Alternatives considered: ACP spec observability extension (deferred, complementary), OpenDray integration (rejected: host-resident model), standalone sidecar (rejected for MVP)

Context: Discord thread discussion on 2026-08-15 (OAB vs OpenDray comparison).

@chaodu-obk
chaodu-obk Bot requested a review from thepagent as a code owner August 15, 2026 02:14
@chaodu-obk

This comment has been minimized.

@chaodu-obk

This comment has been minimized.

@chaodu-obk

chaodu-obk Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Important

CHANGES REQUESTED ⚠️ -- Final consolidated review: the ADR is well-researched and honestly written, but five critical clusters (positioning vs the Thin Bridge pillar, same-pod blast radius, auth below shell-equivalent capability, scrollback self-contradiction, unsupported pool-reuse claims) block acceptance as written. The convergent recommendation is to reshape the decision rather than patch the text.

What This PR Does

Proposes an opt-in PTY mode: a second session backend where OAB spawns a CLI as a PTY subprocess inside its pod and streams the raw terminal bidirectionally over WebSocket, making OAB dual-persona (ACP message broker + terminal server). ADR only -- no code changes.

How It Works

Session-pool reuse (pty:{session_name} keys, pgid kill handles, scrollback ring buffer replayed on reattach), WebSocket upgrade with Bearer token auth on the axum listener, feature-gated (pty) and disabled by default, 4-phase plan, plus a sec. 6 prior-art survey (OpenDray, Herdr, Claude Code cross-session messaging) mapped to implementation phases.

Scope of This Comment

This round-3 comment is the final consolidated review for head 0c8c692a. It supersedes and absorbs the round-1 and round-2 summaries (now minimized) and folds in the full multi-lane group review (correctness, architecture x2, security/CI, docs/UX, operability, infrastructure, simplicity, and four general lanes; the spec-verification lane was covered by independent source checks). There are no inline review comments or external reviewer comments on this PR to respond to; the Review Contract CI failure flagged in earlier rounds has been fixed (see Resolved below).

Findings

# Severity Finding Location
C1 🔴 Positioning drift: the "Terminal Server" persona conflicts with DESIGN.md pillar #1 ("thin bridge" as a deliberate non-decision); the ADR asserts coexistence without amending or reconciling it. The observability motivation (sec. 1) and the direct-keyboard-control decision (sec. 2) are different problems; Alternative A addresses the stated pain and is never compared head-to-head; an "Alternative E" (remain a pure broker; let existing terminal tools own this) is absent; no demand evidence beyond one discussion thread sec. 1/2/4 vs DESIGN.md
C2 🔴 Same-pod blast radius: "sandbox posture unchanged" does not hold. env_clear only limits inherited environment; an interactive shell in the broker pod shares PID/cgroup/network namespaces and the mounted credential plane, can signal the broker, exhaust shared resources, and use full pod egress sec. 2 Security model
C3 🔴 Auth contract below the capability it protects: single static shared key with no WSS mandate, no fail-closed bind, no rotation/expiry/revocation, no per-session binding, no identity layer; browsers cannot set an Authorization header on WebSocket upgrade (the existing /acp endpoint solves this with a bearer-carrying Sec-WebSocket-Protocol offer); OPENAB_PTY_AUTH_KEY uses the raw env-var pattern that docs/adr/secrets-management.md explicitly flags, without referencing it sec. 2 vs acp_server.rs, secrets-management.md, identity-trust-none.md
C4 🔴 Scrollback self-contradiction: sec. 6 claims Herdr's "replay off by default -- scrollback may contain secrets" lesson is adopted, while sec. 5 Phase 1 ships unconditional replay-on-attach; retention, bounds, teardown clearing, and non-persistence are unspecified sec. 5 Phase 1 vs sec. 6
C5 🔴 Pool-reuse claims unsupported by the codebase: PoolState is ACP-specific (connection handles, ACP session IDs, session/load persistence, JSON-RPC cancel); the claimed "idle TTL paused while attached" mechanism does not exist in the pool today; ACP turn-based liveness and PTY byte-stream liveness are incompatible, so "a new backend, not a parallel pool" is not an available boundary without a designed abstraction or a separate pool sec. 2 vs crates/openab-core/src/acp/pool.rs
I1 🟡 PTY lifecycle contract undefined: what counts as attached, half-open WS liveness, no absolute lifetime cap (attached-pause is an eviction bypass and standing-access risk), interaction with hung detection sec. 5 Phase 1
I2 🟡 Cross-persona capacity isolation missing: shared max_sessions with no quota, reservation, fairness, or per-token limits; ACP and PTY eviction costs are asymmetric (ACP can suspend/resume; PTY kill loses interactive state) sec. 2 / sec. 5
I3 🟡 Reconnect/replay contract inconsistent: "replay then resubscribe" races (bytes lost or duplicated) without an atomic snapshot + monotonic cursor + gap signaling; sec. 6 assigns the cursor to Phase 1 while sec. 5 defers it to Phase 3 sec. 5 vs sec. 6
I4 🟡 Concurrent attach undefined in MVP: must choose single-attach-exclusive or a writer-lease protocol; with one shared token every reader is also a writer sec. 5 Phase 1 vs 3
I5 🟡 API surface too thin: no session create/kill in Phase 1 (attach-only has no bootstrap path), no control-frame schema, no error-code mapping, no name-ownership/collision rules sec. 2 / sec. 5
I6 🟡 Session naming and command authority unspecified: no name validation (path traversal, key injection); operator-configured vs client-specified command is a critical security distinction left ambiguous ("anything on PATH" implies client freedom) sec. 2 / sec. 5
I7 🟡 PTY env allowlist needed: env_clear exists only in the ACP spawn path, and an interactive TTY needs TERM/LANG-class variables; must forbid inheriting OPENAB_* and cloud credentials sec. 2
I8 🟡 Resource bounds and backpressure unspecified: frame size, per-client queues, slow-consumer policy, scrollback caps sec. 2 / sec. 5
I9 🟡 Process-group/fd teardown invariants not carried over: setpgid on spawn, escalating kill, evict-while-attached ordering, child-exit cleanup sec. 2
I10 🟡 Dead-process recovery and pod-restart taxonomy missing: "session survives the client" holds only while the pod lives; reattach (process alive) vs recover (process dead) are conflated sec. 2 / sec. 5
I11 🟡 Runtime disable/rollback drain path undefined for live PTY sessions sharing the broker process sec. 2
I12 🟡 MVP operational security deferred: attach/detach and auth-failure audit, rate limiting, and basic metrics belong in Phase 1 for a shell-granting endpoint sec. 5
I13 🟡 K8s constraints unexamined: NetworkPolicy/egress (a PTY user gets full pod egress), resource requests/limits, readOnlyRootFilesystem compatibility, and listener topology -- a separate listener/port should be considered so network policy can isolate shell access from platform-webhook traffic sec. 2 + chart
I14 🟡 Feature flag wiring inaccurate: dep:axum provides no listener and no core-crate feature leg; pool-shape complexity exists even when the feature is compiled out sec. 2 / Cargo.toml
I15 🟡 Sec. 6 adopt-in mappings conflict with sec. 5: terminal-capability filtering marked Phase 1 but absent from Phase 1 bullets (a known startup-breakage risk for Ink-based CLIs); "Phase 1 API design" / "Phase 1 auth design" targets have no anchor in the plan sec. 5 vs sec. 6
I16 🟡 A2A scope creep: "A2A semantics" used but never defined; UDS own-child verification and callback env injection introduce agent-to-agent capability that contradicts the single-human scope and conflates two trust models sec. 6 vs sec. 1/2
I17 🟡 Survey depth/citation asymmetry: exact upstream file paths will rot; OpenDray rows cite sources while others do not; the JSONL-transcript row targets the same gap as Alternative A without cross-reference sec. 6
I18 🟡 Relationship to the existing AgentCore remote-PTY path is not addressed; needs a non-goals statement (when to use AgentCore sandbox vs this mode) sec. 1/2 vs docs/agentcore.md
I19 🟡 Phase 4 "waiting on input" has no stated detection mechanism for raw byte streams; scope to an explicit heuristic or defer sec. 5 Phase 4
I20 🟡 Documentation consistency: diagram branch glyphs, OpenAB/OAB naming, missing config-reference forward refs; Related should cite the as-built acp-server-websocket-base.md (Accepted) rather than only the proposal ADR header / various
R1 RESOLVED: Review Contract CI -- the PR body now contains the required ## Review Contract section and the check passes (note for maintainers: the review-contract-exempt label referenced by the validator still does not exist in this repo) PR body / CI
R2 RESOLVED: Herdr reference link added to the References section sec. 7
G1-G5 🟢 See What's Good below --
Finding Details

🔴 C1: Reconcile the product identity before the design details matter

DESIGN.md pillar #1 defines OAB as a deliberately thin transport whose non-decisions are features. A terminal server that owns subprocesses, scrollback, render surfaces, and input streams is a different product category; a sandboxed terminal server is still a terminal server. The ADR also leads with an observability pain that PTY mode bypasses rather than fixes -- Alternative A (ACP shellOutput/commandLog events) addresses that pain inside the protocol boundary and is dismissed in one paragraph without a head-to-head comparison on the actual need. A fifth alternative (remain a pure broker and interoperate with existing terminal tools) is never debated, and no demand evidence is cited.

Requested change: either (a) explicitly amend DESIGN.md pillar #1 and accept the positioning shift, or (b) reshape the decision: a separately deployable PTY runtime (own pod/binary/token, with OAB acting only as an authenticated discovery/notification bridge) plus Alternative A pursued for the in-bridge visibility gap. Add Alternative E and demand evidence either way. The review does not reject the remote-PTY need itself -- it rejects the in-process dual-persona form.

🔴 C2: Define a real isolation boundary

The broker process and the PTY subprocess share one PID namespace, cgroup, network namespace, and mounted credential plane. env_clear does not isolate service-account tokens, provider credential files, same-UID /proc, signals, or egress. A PTY client is an interactive shell inside the broker's security domain, and a compromise reaches every ACP/platform session the pod serves.

Requested change: commit to one of: separate pod per PTY session; sidecar container (separate PID namespace and cgroup); or accept and document same-pod privilege with enumerated compensating controls and a recommendation that PTY-enabled replicas deploy separately from ACP-serving replicas. Remove or rewrite "sandbox posture unchanged".

🔴 C3: Match the auth contract to shell-equivalent capability

Required: WSS/TLS mandate (or a documented TLS-terminating boundary); fail-closed startup when the key is unset; browser-safe credential transport reusing the validated /acp Sec-WebSocket-Protocol scheme with origin policy and constant-time comparison; short-lived or revocable per-session tokens (a static shared key on a mobile device is a permanent pod shell on device loss); separation from the ACP key; sourcing via the secrets resolver per secrets-management.md; and an identity layer or an explicit statement of its absence per identity-trust-none.md.

🔴 C4: Resolve the scrollback contradiction

Make Phase 1 match the claimed adoption: replay off by default or behind explicit opt-in; bounded buffer with a hard cap; cleared on teardown; never persisted to disk; documented as sensitive memory that is not a redaction boundary.

🔴 C5: Decide and cost the pool integration shape

Choose one explicitly: a backend-neutral lifecycle/quota/pgid abstraction with separate ACP and PTY backend state, or a dedicated PTY manager sharing only capacity and termination primitives. State which invariants are shared, which ACP behaviors do not apply, define what "idle" means for an unattached PTY (a detached long-running build is active, not idle), and estimate the regression surface on the shipped ACP path.

🟡 I1-I20

Each item in the table above carries its requested change inline; the fastest path is to treat I1-I14 as the Phase 1 contract checklist (lifecycle, capacity, replay cursor, writer arbitration, create/kill API, naming/command authority, env allowlist, backpressure, teardown, recovery, rollback, audit, K8s posture, feature wiring) and I15-I20 as document-consistency repairs (phase-mapping reconciliation, A2A re-scoping, citation parity, AgentCore non-goals, Phase 4 heuristic, naming/reference cleanups).

Baseline Check
  • PR opened 2026-08-15; head 0c8c692ac9e3ad34b6f04c41f5c330b069dc1c56; base main; merge-base 94354a75; diff: 1 file, +239 (net-new docs/adr/pty-mode.md)
  • Main already has: docs/adr/acp-server-websocket.md (proposal), docs/adr/acp-server-websocket-base.md (as-built), docs/adr/unified-binary.md, docs/adr/secrets-management.md, docs/adr/identity-trust-none.md, DESIGN.md pillar perf: cache dependency build layer in Dockerfile #1, and the ACP session pool
  • Prior-art accuracy independently verified: OpenDray internal/session/ source, Herdr public docs, and Claude Code cross-session messaging docs all check out (URLs, versions, state taxonomies, throttling constants)
  • Review coverage: thirteen lanes reported (correctness, architecture x2, security/CI, docs/UX x2, operability, infrastructure, simplicity, and four general lanes); verdicts were unanimous
  • External feedback: no inline review comments, submitted reviews, or non-team comments exist on this PR; nothing to resolve
What's Good (🟢)
  • G1: Honest trust-boundary framing -- "token possession = pod shell access" is the correct threat-model statement most PTY-over-WS designs omit
  • G2: Defense-in-depth enablement -- compile-time feature gate AND runtime flag, both default-off, with an auth key independent from platform tokens
  • G3: ADR structure and conventions match the corpus precisely; all cross-references to sibling ADRs and source files check out
  • G4: The sec. 6 prior-art survey is accurate, vendor-specific, and phase-mapped -- reusable as-is in any follow-up ADR regardless of the direction chosen
  • G5: The quadrant framing and alternatives section genuinely engage rejected options rather than strawmanning them

5. Three Reasons We Might Not Need This PR

  1. The quadrant may be empty for a reason -- users who want raw terminals tend not to want sandboxes, and users who want sandboxes tend to want the structured ACP flow; the demand is asserted, not evidenced.
  2. The observability motivation is solvable inside the protocol -- ACP spec evolution (Alternative A) serves every existing client without a second persona, a second trust model, or a second client surface.
  3. The dual-persona tax compounds while the reuse savings do not -- two liveness definitions, two auth models, two client surfaces, and two incident models in one binary, versus a separate runtime that can ship, evolve, and be deprecated independently.

Disposition

Verdict: CHANGES REQUESTED -- the ADR should not be accepted as written. The maintainer will choose among: full revision in place, trimming to a Phase-1-only bounded experiment, replacing with a separately-deployable PTY runtime ADR (the review's convergent recommendation), or closing in favor of an ACP observability ADR. The sec. 6 research is portable to any of these outcomes.

@chaodu-obk

chaodu-obk Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Closing per maintainer decision after the consolidated review above: the in-process dual-persona form is superseded by an optional openab-pty sidecar runtime proposal, which resolves the five blocker clusters by construction (container boundary, per-session token model, secrets-safe scrollback default, dedicated PTY session manager, default-off reversible deployment) while keeping the OAB broker binary untouched.

Successor: #1478

The prior-art survey (sec. 6) carries over to the new ADR with adopt-in targets normalized against its phase plan. The review record here remains the rationale for rejecting the in-process form.

@chaodu-obk chaodu-obk Bot closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant