tasks(tty): decompose alknet-tty + alknet-tty-local into 15 implementation tasks
Break the tty spec set (docs/architecture/crates/tty/) into atomic,
dependency-ordered tasks across two crates:
alknet-tty (lean core, depends on alknet-core only — ADR-057):
- crate-init, wire-codec, control-messages, negotiation, backend-trait,
adapter, review-tty
alknet-tty-local (sibling, portable_pty + libc — ADR-054):
- crate-init, pty-mode, pipe-mode, backend-impl, review-tty-local
integration:
- local-feature-reexport, integration-test, review-tty-final
Three review checkpoints at critical points: after the core crate (before
the local backend builds on the one-way-door TtyBackend trait — ADR-053),
after the local backend (validating REQ-TTY-01/02 and ADR-056), and a final
merge-readiness review. The high-risk tasks (backend-trait, adapter,
pty-mode) are flagged; the ADR-056 kill-on-Drop guard the POC lacked is
called out in both pty-mode and pipe-mode.
Validated with taskgraph: 115 tasks, no cycles, 10-generation parallel
structure with 3-way parallelism after crate-init and 2-way after the
backend trait.
This commit is contained in:
glm-5.2 committed
1 parent
dbac972aaa
commit
8d20f89902
15 files changed
+2409
No files matched your search
@@ -0,0 +1,114 @@
|
||||
---
|
||||
id: tty-local/backend-impl
|
||||
name: Implement LocalTtyBackend (TtyBackend) branching on terminal Some/None
|
||||
status: pending
|
||||
depends_on: [tty-local/pty-mode, tty-local/pipe-mode]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement `LocalTtyBackend` in `src/backend.rs`: the `TtyBackend` implementation
|
||||
that branches on `TtyParams.terminal` to dispatch to PTY mode (task
|
||||
`tty-local/pty-mode`) or pipe mode (task `tty-local/pipe-mode`).
|
||||
|
||||
### LocalTtyBackend
|
||||
|
||||
```rust
|
||||
pub struct LocalTtyBackend;
|
||||
|
||||
impl LocalTtyBackend {
|
||||
pub fn new() -> Self;
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl TtyBackend for LocalTtyBackend {
|
||||
async fn allocate(&self, params: &TtyParams) -> Result<TtyHandle, TtyError> {
|
||||
match ¶ms.terminal {
|
||||
Some(terminal) => pty::allocate_pty(
|
||||
terminal.clone(),
|
||||
params.cmd.clone(),
|
||||
params.cwd.clone(),
|
||||
params.env.clone(),
|
||||
).await,
|
||||
None => pipe::allocate_pipe(
|
||||
params.cmd.clone(),
|
||||
params.cwd.clone(),
|
||||
params.env.clone(),
|
||||
).await,
|
||||
}
|
||||
}
|
||||
|
||||
fn resource_id(&self, _params: &TtyParams) -> Option<(&'static str, String)> {
|
||||
None // local backend creates its own resource (process); no pre-existing resource
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`LocalTtyBackend` takes no constructor dependencies (unlike
|
||||
`DockerTtyBackend` which wraps a `bollard::Docker` client). The
|
||||
`portable_pty` system is process-global. The assembly layer constructs one
|
||||
`LocalTtyBackend` and registers it as `"local"`.
|
||||
|
||||
### Validation
|
||||
|
||||
`allocate()` validates `params.cmd` is non-empty (the adapter already checks
|
||||
this at negotiation, but the backend should fail gracefully if called
|
||||
directly with an empty cmd — return `TtyError::AllocFailed`). The
|
||||
`backend_params` map is ignored by the local backend (it has no
|
||||
backend-specific selector fields); if a caller passes unexpected
|
||||
`backend_params`, they are silently ignored (the local backend doesn't
|
||||
define a typed params struct).
|
||||
|
||||
### Default trait methods
|
||||
|
||||
`resource_id()` returns `None` (the local backend creates its own resource —
|
||||
a process — so there's no pre-existing resource for the ownership check). The
|
||||
default impl on `TtyBackend` already returns `None`, so this can be omitted
|
||||
unless an explicit impl is preferred for clarity.
|
||||
|
||||
### Tests
|
||||
|
||||
- **PTY dispatch**: `allocate` with `terminal: Some` → returns a `TtyHandle`
|
||||
with `stderr: None` and a `PtyControl`.
|
||||
- **Pipe dispatch**: `allocate` with `terminal: None` → returns a `TtyHandle`
|
||||
with `stderr: Some` and a `PipeControl`.
|
||||
- **Empty cmd**: `allocate` with empty `cmd` → `TtyError::AllocFailed`.
|
||||
- **resource_id**: returns `None`.
|
||||
- These can be lightweight (the heavy tests are in `pty-mode` and `pipe-mode`);
|
||||
this task verifies the dispatch wiring.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `LocalTtyBackend` struct with `new()` constructor (no deps)
|
||||
- [ ] `impl TtyBackend for LocalTtyBackend` with `allocate()` branching on `params.terminal`
|
||||
- [ ] `terminal: Some` → dispatches to `pty::allocate_pty`
|
||||
- [ ] `terminal: None` → dispatches to `pipe::allocate_pipe`
|
||||
- [ ] `resource_id()` returns `None` (or uses the default)
|
||||
- [ ] Empty `cmd` → `TtyError::AllocFailed`
|
||||
- [ ] `LocalTtyBackend` re-exported from `lib.rs` (`pub use backend::LocalTtyBackend`)
|
||||
- [ ] Unit tests: PTY dispatch, pipe dispatch, empty cmd, resource_id
|
||||
- [ ] `cargo test -p alknet-tty-local` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty-local` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-local.md — §"PTY Mode", §"Pipe Mode", §"LocalTtyBackend takes no constructor dependencies"
|
||||
- docs/architecture/crates/tty/tty-backend.md — `TtyBackend` trait, `resource_id()`
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md — ADR-054 (per-session PTY vs pipe choice)
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the wiring task — the heavy lifting is in `pty-mode` and
|
||||
> `pipe-mode`. The branch on `TtyParams.terminal` is the per-session choice
|
||||
> (ADR-054): one `LocalTtyBackend` serves both terminal and runner use cases.
|
||||
> The local backend ignores `backend_params` (no backend-specific selector
|
||||
> fields); a docker/SSH backend would deserialize its own typed params from
|
||||
> it.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,116 @@
|
||||
---
|
||||
id: tty-local/crate-init
|
||||
name: Initialize alknet-tty-local crate with Cargo.toml, dependencies, and module skeleton
|
||||
status: pending
|
||||
depends_on: [tty/backend-trait]
|
||||
scope: moderate
|
||||
risk: low
|
||||
impact: project
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Initialize the `alknet-tty-local` sibling crate (ADR-054). This crate
|
||||
implements `TtyBackend` for `LocalTtyBackend` via `portable_pty` (PTY mode)
|
||||
and `std::process::Command` (pipe/runner mode). It depends on `alknet-tty`
|
||||
for the trait and types; the heavy `portable_pty` dependency lives here, not
|
||||
in the core crate.
|
||||
|
||||
### Crate setup
|
||||
|
||||
Create `crates/alknet-tty-local/` with:
|
||||
|
||||
- `Cargo.toml` — package metadata, dependencies
|
||||
- `src/lib.rs` — crate root with module declarations and re-exports
|
||||
- Module skeleton files for:
|
||||
- `src/pty.rs` — PTY mode: three-thread bridge, `PtyControl`, REQ-TTY-02
|
||||
signal forwarding, ADR-056 kill guard
|
||||
- `src/pipe.rs` — pipe mode: `tokio::process`, `PipeControl`, ADR-056 kill
|
||||
guard
|
||||
- `src/backend.rs` — `LocalTtyBackend` implementing `TtyBackend`, branching
|
||||
on `TtyParams.terminal`
|
||||
|
||||
### Dependencies
|
||||
|
||||
Per the architecture spec (tty-local.md §"Dependencies"):
|
||||
|
||||
| Crate | Purpose |
|
||||
|-------|---------|
|
||||
| `alknet-tty` | `TtyBackend` trait, `TtyHandle`, `TtyControl`, `TtyControlHandle`, `TtyParams`, `TerminalParams`, `TtyError`, `ControlMessage`, `signal_from_name` (workspace path) |
|
||||
| `portable_pty` | PTY allocation — Unix `openpty` + Windows ConPTY (the heavy dep) |
|
||||
| `libc` | Signal forwarding — REQ-TTY-02, Unix only |
|
||||
| `tokio` 1 (full) | mpsc, oneshot, `AsyncRead`/`AsyncWrite` for the pipe case |
|
||||
| `bytes` 1 | `Bytes` for stdout/stderr streams |
|
||||
| `futures-core` | `Stream` trait for `TtyHandle.stdout`/`stderr` |
|
||||
| `tracing` 0.1 | Structured logging |
|
||||
| `thiserror` 2 | Error conversion (if needed beyond `TtyError`) |
|
||||
|
||||
`alknet-tty-local` does NOT depend on `alknet-core` directly (it accesses core
|
||||
types via `alknet-tty`'s re-exports if needed). `portable_pty` and `libc` are
|
||||
the heavy deps that motivate the sibling-crate split (ADR-054).
|
||||
|
||||
### Feature flags
|
||||
|
||||
No feature flags — the crate is unconditional. The `local` feature gate lives
|
||||
on `alknet-tty` (the consumer enables it to pull this crate in).
|
||||
|
||||
### Workspace Cargo.toml
|
||||
|
||||
Add `crates/alknet-tty-local` to the workspace `members` list in the root
|
||||
`Cargo.toml`.
|
||||
|
||||
### Module skeleton
|
||||
|
||||
```rust
|
||||
// src/lib.rs
|
||||
//! alknet-tty-local: Local TTY backend for alknet-tty.
|
||||
//!
|
||||
//! `LocalTtyBackend` implements `alknet_tty::TtyBackend` via `portable_pty`
|
||||
//! (PTY mode, terminal semantics) and `std::process::Command` (pipe mode,
|
||||
//! the runner case). The blocking→async bridge for PTY mode uses three
|
||||
//! dedicated std threads feeding tokio mpsc/oneshot channels (REQ-TTY-01).
|
||||
//! Signal forwarding targets the foreground process group (REQ-TTY-02).
|
||||
|
||||
pub mod pty;
|
||||
pub mod pipe;
|
||||
pub mod backend;
|
||||
|
||||
pub use backend::LocalTtyBackend;
|
||||
```
|
||||
|
||||
Each module file gets a doc comment and `// TODO: implement` marker.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `crates/alknet-tty-local/Cargo.toml` exists with all dependencies
|
||||
- [ ] `crates/alknet-tty-local/src/lib.rs` exists with module declarations and `pub use backend::LocalTtyBackend`
|
||||
- [ ] Module skeleton files exist: `pty.rs`, `pipe.rs`, `backend.rs`
|
||||
- [ ] Root `Cargo.toml` `members` list includes `crates/alknet-tty-local`
|
||||
- [ ] `cargo check -p alknet-tty-local` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty-local` succeeds with no warnings
|
||||
- [ ] Dual licensing: `MIT OR Apache-2.0` (workspace-inherited)
|
||||
- [ ] `alknet-tty` dependency uses workspace path (`path = "../alknet-tty"`)
|
||||
- [ ] `portable_pty` and `libc` dependencies present
|
||||
- [ ] No `alknet-core` direct dependency (accessed via `alknet-tty` if needed)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-local.md — the authoritative spec for this crate
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md — ADR-054 (sibling crate placement)
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md — ADR-053 (the trait this crate implements)
|
||||
- /workspace/alknet-tty-poc/src/local_pty.rs — the reference PTY implementation
|
||||
|
||||
## Notes
|
||||
|
||||
> This crate can be initialized in parallel with the remaining core crate
|
||||
> tasks (`tty/control-messages`, `tty/negotiation`, `tty/adapter`) since it
|
||||
> only depends on `tty/backend-trait` for the trait and types. The
|
||||
> `portable_pty` dependency is the heavy dep that motivates the sibling-crate
|
||||
> split — a docker-only deployment doesn't pull it in. The POC's
|
||||
> `local_pty.rs` is the reference for the PTY mode; the pipe mode is simpler
|
||||
> (tokio's `Child` is natively async).
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,185 @@
|
||||
---
|
||||
id: tty-local/pipe-mode
|
||||
name: Implement pipe mode (tokio::process, PipeControl, ADR-056 kill guard)
|
||||
status: pending
|
||||
depends_on: [tty-local/crate-init]
|
||||
scope: moderate
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the pipe mode in `src/pipe.rs`: the runner case (`terminal: None`,
|
||||
no PTY). Spawn the command with `tokio::process::Command` (or
|
||||
`std::process::Command` with `Stdio::piped()`) for stdin/stdout/stderr, return
|
||||
a `TtyHandle` with separate stdout and stderr (stderr is `Some`) and a
|
||||
`PipeControl` whose `resize()` is a no-op (no PTY) and `signal()` calls
|
||||
`libc::kill(pid, sig)` on the child's pid.
|
||||
|
||||
The async bridge is simpler than the PTY case — tokio's `Child` provides
|
||||
`AsyncRead` for stdout/stderr and `AsyncWrite` for stdin directly (no
|
||||
std-thread bridge needed).
|
||||
|
||||
### Pipe mode (`terminal: None`)
|
||||
|
||||
`allocate_pipe` spawns the command with `Stdio::piped()` for stdin, stdout,
|
||||
and stderr. `TtyHandle.stderr` is `Some` (separate streams). The `exit_code`
|
||||
future is `Child::wait()` (async on tokio's `Child`).
|
||||
|
||||
### Types
|
||||
|
||||
```rust
|
||||
pub fn allocate_pipe(
|
||||
cmd: Vec<String>,
|
||||
cwd: Option<PathBuf>,
|
||||
env: HashMap<String, String>,
|
||||
) -> Result<TtyHandle, TtyError>;
|
||||
```
|
||||
|
||||
- `TtyHandle.stdin` — `Child::stdin` (`ChildStdin` implements `AsyncWrite`),
|
||||
boxed as `Box<dyn AsyncWrite + Send + Unpin>`.
|
||||
- `TtyHandle.stdout` — `Child::stdout` wrapped as a `Stream<Item = Bytes>`.
|
||||
Use `tokio_util::io::ReaderStream` or a manual `AsyncRead`→`Stream` adapter
|
||||
to convert `ChildStdout` (`AsyncRead`) into `Pin<Box<dyn Stream<Item = Bytes> + Send>>`.
|
||||
- `TtyHandle.stderr` — `Some`, same wrapping as stdout.
|
||||
- `TtyHandle.exit_code` — a `Future` wrapping `Child::wait()` PLUS a kill
|
||||
guard (ADR-056, see below).
|
||||
- `TtyHandle.control` — `Some(TtyControlHandle::new(Arc::new(PipeControl)))`.
|
||||
|
||||
### PipeControl (implements TtyControl)
|
||||
|
||||
```rust
|
||||
pub struct PipeControl {
|
||||
pid: Option<u32>,
|
||||
}
|
||||
|
||||
impl TtyControl for PipeControl {
|
||||
fn resize(&self, _cols: u16, _rows: u16, _pw: u16, _ph: u16) {
|
||||
// No-op — no PTY in pipe mode.
|
||||
}
|
||||
fn signal(&self, name: &str) {
|
||||
// libc::kill(pid, sig) on the child's pid (NOT the process group —
|
||||
// pipe mode has no session leader / controlling tty).
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`resize()` is a no-op (no PTY — resize doesn't apply). `signal()` calls
|
||||
`libc::kill(pid, sig)` on the child's pid. Signal forwarding to the process
|
||||
group is **not applicable** in pipe mode (there's no session leader /
|
||||
controlling tty); `kill(pid, sig)` reaches the direct child only. If the child
|
||||
has spawned its own children, they won't receive the signal — this is a known
|
||||
limitation of the runner case (a runner that needs process-group signal
|
||||
delivery uses the PTY case, not the pipe case). Document this in the
|
||||
`PipeControl::signal` doc comment.
|
||||
|
||||
Use `alknet_tty::signal_from_name` for the name→number mapping. Unknown names
|
||||
fall back to `Child::start_kill()` (SIGKILL) — there's no `ChildKiller` in
|
||||
pipe mode; `tokio::process::Child::start_kill()` is the kill path. On non-Unix,
|
||||
`signal()` calls `start_kill()` directly (no `libc::kill`).
|
||||
|
||||
### Cancel-Cleanup (ADR-056)
|
||||
|
||||
The `exit_code` future's `Drop`-on-cancel MUST kill the child. Implement a
|
||||
`PipeExitFuture` wrapping the `Child::wait()` future plus a kill guard holding
|
||||
the `tokio::process::Child` handle:
|
||||
|
||||
```rust
|
||||
struct PipeExitFuture {
|
||||
wait: Pin<Box<dyn Future<Output = Result<ExitStatus, io::Error>> + Send>>,
|
||||
child: Option<tokio::process::Child>, // None after resolve (disarmed)
|
||||
}
|
||||
|
||||
impl Future for PipeExitFuture {
|
||||
// poll delegates to wait; on Ready, take child (disarm), map ExitStatus to i32
|
||||
}
|
||||
|
||||
impl Drop for PipeExitFuture {
|
||||
fn drop(&mut self) {
|
||||
if let Some(mut child) = self.child.take() {
|
||||
let _ = child.start_kill(); // best-effort; child may already be exiting
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
On cancel: `Drop` calls `start_kill()` (SIGKILL); the child exits; the `wait`
|
||||
future reaps it. On happy path: the future resolves, the guard is disarmed,
|
||||
the subsequent `Drop` is a no-op.
|
||||
|
||||
**Critical**: a bare `Child::wait()` future without the kill guard violates
|
||||
the ADR-056 contract and will orphan processes on cancel. This task MUST add
|
||||
the guard. Note: `tokio::process::Child` must be created with
|
||||
`kill_on_drop(true)` OR the explicit guard — the guard is the spec-compliant
|
||||
mechanism; `kill_on_drop` alone is insufficient because the `Child` is moved
|
||||
into the future and the future's `Drop` is the cancel path, not the `Child`'s.
|
||||
Use the explicit guard.
|
||||
|
||||
### The Threading/Deadlock Caveat (DP-4)
|
||||
|
||||
`std::process::Command` with piped stdio can deadlock if stdin writes block
|
||||
while stdout/stderr buffers fill — the classic pipe-buffer deadlock. The fix
|
||||
is concurrent reads on stdout/stderr alongside stdin writes, which is exactly
|
||||
what the adapter's three-pump driver does (task `tty/adapter`). No design
|
||||
decision needed; the spec notes it as a known constraint with a known
|
||||
(POC-validated) solution.
|
||||
|
||||
### Tests
|
||||
|
||||
- **Happy path**: spawn `echo hello` in pipe mode, read stdout stream to EOF,
|
||||
await exit_code → 0, assert stderr is empty.
|
||||
- **Stdin round-trip**: spawn `cat`, write via `AsyncWrite` stdin, read back
|
||||
via stdout, close stdin, await exit 0.
|
||||
- **Separate stderr**: spawn a command that writes to stderr (e.g.,
|
||||
`sh -c "echo err >&2"`), assert stderr stream receives the bytes, stdout
|
||||
is empty.
|
||||
- **Signal (Unix)**: spawn `sleep 60`, send `signal("TERM")`, assert the child
|
||||
exits with signal-terminated status.
|
||||
- **Cancel cleanup (ADR-056)**: spawn `sleep 60`, drop the `TtyHandle` without
|
||||
awaiting exit_code, assert the child is killed (no orphan).
|
||||
- **Resize no-op**: call `PipeControl::resize`, assert no error (it's a no-op).
|
||||
- **Unknown signal name**: send `signal("NOSUCH")`, assert it falls back to
|
||||
`start_kill()` (SIGKILL).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `allocate_pipe` function spawns via `tokio::process::Command` with `Stdio::piped()`
|
||||
- [ ] `TtyHandle.stdin` is `ChildStdin` boxed as `Box<dyn AsyncWrite + Send + Unpin>`
|
||||
- [ ] `TtyHandle.stdout` is `ChildStdout` wrapped as `Pin<Box<dyn Stream<Item = Bytes> + Send>>`
|
||||
- [ ] `TtyHandle.stderr` is `Some` (same wrapping as stdout)
|
||||
- [ ] `TtyHandle.exit_code` is `PipeExitFuture` (wait + kill guard), `BoxFuture<Result<i32, TtyError>>`
|
||||
- [ ] `TtyHandle.control` is `Some(TtyControlHandle::new(Arc::new(PipeControl)))`
|
||||
- [ ] `PipeControl` implements `alknet_tty::TtyControl`
|
||||
- [ ] `PipeControl::resize` is a no-op
|
||||
- [ ] `PipeControl::signal` uses `libc::kill(pid, sig)` (Unix) / `start_kill()` (non-Unix); NOT process-group targeting
|
||||
- [ ] `PipeControl::signal` uses `alknet_tty::signal_from_name`; unknown names fall back to `start_kill()`
|
||||
- [ ] `PipeExitFuture::Drop` calls `Child::start_kill()` when dropped without resolving (ADR-056)
|
||||
- [ ] `PipeExitFuture` disarms the guard on resolve (no-op Drop on happy path)
|
||||
- [ ] Doc comment on `PipeControl::signal` notes the no-process-group limitation
|
||||
- [ ] Integration tests: happy path, stdin round-trip, separate stderr, signal, cancel cleanup, resize no-op, unknown signal
|
||||
- [ ] `cargo test -p alknet-tty-local` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty-local` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-local.md — §"Pipe Mode (`terminal: None`)", §"Cancel-Cleanup (ADR-056)", §"The Threading/Deadlock Caveat"
|
||||
- docs/architecture/crates/tty/tty-backend.md — `TtyHandle`, `TtyControl` (the types this produces)
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md — ADR-056 (kill-on-Drop)
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md — ADR-054 (pipe mode = runner case)
|
||||
|
||||
## Notes
|
||||
|
||||
> Pipe mode is simpler than PTY mode (tokio's `Child` is natively async, no
|
||||
> std-thread bridge). The two subtleties: (1) the `AsyncRead`→`Stream<Item=Bytes>`
|
||||
> conversion for stdout/stderr (use `tokio_util::io::ReaderStream` or a manual
|
||||
> adapter), and (2) the ADR-056 kill guard on the `exit_code` future. Do NOT
|
||||
> rely on `tokio::process::Child::kill_on_drop(true)` alone — the explicit
|
||||
> guard in the future's `Drop` is the spec-compliant mechanism. Signal
|
||||
> forwarding in pipe mode targets the direct child only (no process group);
|
||||
> document this limitation.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,232 @@
|
||||
---
|
||||
id: tty-local/pty-mode
|
||||
name: Implement PTY mode (three-thread bridge, PtyControl, REQ-TTY-02, ADR-056 kill guard)
|
||||
status: pending
|
||||
depends_on: [tty-local/crate-init]
|
||||
scope: broad
|
||||
risk: high
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the PTY mode in `src/pty.rs`: allocate a real PTY via
|
||||
`portable_pty::native_pty_system().openpty()`, spawn the command into the
|
||||
slave side, and return a `TtyHandle` with merged stdout (stderr is `None` —
|
||||
kernel PTY property) and a real `TtyControl` (resize via `MasterPty::resize`,
|
||||
signal via `libc::kill(-pgid, sig)`).
|
||||
|
||||
This is the reference implementation of REQ-TTY-01 (backends need not be
|
||||
natively async) and carries REQ-TTY-02 (signal forwarding to the process
|
||||
group). It is a port + generalization of the POC's
|
||||
`/workspace/alknet-tty-poc/src/local_pty.rs`, adapted to produce a `TtyHandle`
|
||||
(the trait's handle type) instead of the POC's `LocalPty` struct.
|
||||
|
||||
### The blocking→async bridge (REQ-TTY-01)
|
||||
|
||||
`portable_pty` is a blocking `std::io` API. `MasterPty::try_clone_reader()`
|
||||
returns `Box<dyn std::io::Read + Send>`; `take_writer()` returns
|
||||
`Box<dyn std::io::Write + Send>`; `Child::wait()` blocks. Bridge to async via
|
||||
**three dedicated std threads** feeding tokio mpsc/oneshot channels:
|
||||
|
||||
1. **Reader thread** — blocking reads from `MasterPty::try_clone_reader()` →
|
||||
`mpsc::Sender<Bytes>`. Reads into an 8 KiB buffer, copies each chunk to
|
||||
`Bytes`, `blocking_send`s to the mpsc. On EOF (master reader returns EOF
|
||||
when the slave closes — child exited, PTY buffer drained), sends a
|
||||
zero-length `Bytes` sentinel and exits. The async-facing `TtyHandle.stdout`
|
||||
is the `mpsc::Receiver<Bytes>` wrapped as `Pin<Box<dyn Stream<Item = Bytes> + Send>>`.
|
||||
2. **Writer thread** — drains an `mpsc::Receiver<StdinCmd>` → blocking writes
|
||||
to `MasterPty::take_writer()`. `StdinCmd::Bytes(bytes)` writes and flushes;
|
||||
`StdinCmd::Eof` drops the writer (EOF to slave stdin) and exits. The
|
||||
async-facing `TtyHandle.stdin` is the `mpsc::Sender<StdinCmd>` wrapped as
|
||||
`Box<dyn AsyncWrite + Send + Unpin>` (an `AsyncWrite` impl that wraps each
|
||||
`write` as a `StdinCmd::Bytes` and `flush` as a no-op).
|
||||
3. **Waiter thread** — blocking `Child::wait()` → `oneshot::Sender<i32>` with
|
||||
the exit code. The async-facing `TtyHandle.exit_code` is a `Future` wrapping
|
||||
this `oneshot::Receiver<i32>` PLUS a kill guard holding the
|
||||
`portable_pty::ChildKiller` (see ADR-056 below).
|
||||
|
||||
`TtyHandle.stderr` is `None` (PTY backends merge stdout/stderr).
|
||||
|
||||
### StdinCmd
|
||||
|
||||
```rust
|
||||
pub enum StdinCmd {
|
||||
Bytes(Vec<u8>), // write these bytes to the master writer
|
||||
Eof, // close the master writer (EOF to the slave's stdin)
|
||||
}
|
||||
```
|
||||
|
||||
### AsyncWrite wrapper for stdin
|
||||
|
||||
Implement an `AsyncWrite` impl over `mpsc::Sender<StdinCmd>`: `poll_write`
|
||||
sends `StdinCmd::Bytes(bytes.to_vec())` (awaiting the send via `poll`), `poll_flush`
|
||||
is a no-op (the writer thread flushes), `poll_close` sends `StdinCmd::Eof`. This
|
||||
wraps the tokio mpsc sender as the `Box<dyn AsyncWrite + Send + Unpin>` the
|
||||
`TtyHandle.stdin` field requires.
|
||||
|
||||
### PtyControl (implements TtyControl)
|
||||
|
||||
```rust
|
||||
pub struct PtyControl {
|
||||
master: Arc<Mutex<Box<dyn MasterPty + Send>>>,
|
||||
killer: Arc<Mutex<Box<dyn portable_pty::ChildKiller + Send + Sync>>>,
|
||||
pid: Option<u32>,
|
||||
}
|
||||
|
||||
impl TtyControl for PtyControl {
|
||||
fn resize(&self, cols: u16, rows: u16, pixel_width: u16, pixel_height: u16);
|
||||
fn signal(&self, name: &str);
|
||||
}
|
||||
```
|
||||
|
||||
- `resize()` locks the master and calls `MasterPty::resize(PtySize)` —
|
||||
non-blocking (issues an `ioctl`).
|
||||
- `signal()` — see REQ-TTY-02 below.
|
||||
|
||||
### REQ-TTY-02: Signal Forwarding Must Target the Process Group
|
||||
|
||||
`libc::kill(pid, sig)` on the spawned child's pid alone is **insufficient** for
|
||||
terminal semantics: a shell under a PTY will have spawned children (a
|
||||
`find | grep` pipeline, a `make` with sub-makes), and those children won't
|
||||
receive the signal. A real terminal forwards Ctrl-C to the **foreground
|
||||
process group**.
|
||||
|
||||
`portable_pty` makes the child a session leader (when `controlling_tty = true`,
|
||||
the default — `CommandBuilder::set_controlling_tty(true)`), so the child's pid
|
||||
*is* its process-group id, and `libc::kill(-pid, sig)` (the negative pid)
|
||||
reaches the whole group. The POC's `PtyControl::signal` uses exactly this —
|
||||
`kill(-pgid, sig)` with a fallback to `kill(pid, sig)` if the group signal
|
||||
fails (e.g., the child already exited).
|
||||
|
||||
The spec records (tty-local.md §"REQ-TTY-02"):
|
||||
1. The local backend MUST forward signals to the child's process group, not
|
||||
just the child pid. Using `kill(-pgid, sig)` when the child is a session
|
||||
leader.
|
||||
2. The local backend MUST spawn the child as a session leader with a
|
||||
controlling tty (`CommandBuilder::set_controlling_tty(true)` — the default).
|
||||
3. The `TtyControl::signal` contract is "best-effort delivery to the
|
||||
foreground process group." Unknown signal names fall back to the backend's
|
||||
default kill (`portable_pty`'s `ChildKiller::kill` sends SIGHUP); known
|
||||
names map to `libc` signal numbers via `alknet_tty::signal_from_name` and
|
||||
are sent to the group.
|
||||
|
||||
Use `alknet_tty::signal_from_name` (from `alknet-tty`'s `control` module) for
|
||||
the name→number mapping. On non-Unix, fall back to `ChildKiller::kill`.
|
||||
|
||||
### Cancel-Cleanup (ADR-056)
|
||||
|
||||
The `exit_code` future's `Drop`-on-cancel MUST kill the child. Implement a
|
||||
`LocalExitFuture` wrapping the `oneshot::Receiver<i32>` plus a kill guard:
|
||||
|
||||
```rust
|
||||
struct LocalExitFuture {
|
||||
rx: oneshot::Receiver<i32>,
|
||||
killer: Option<portable_pty::ChildKiller>, // None after resolve (disarmed)
|
||||
}
|
||||
|
||||
impl Future for LocalExitFuture {
|
||||
// poll delegates to rx; on Ready, take killer (disarm)
|
||||
}
|
||||
|
||||
impl Drop for LocalExitFuture {
|
||||
fn drop(&mut self) {
|
||||
if let Some(killer) = self.killer.take() {
|
||||
let _ = killer.kill(SIGHUP); // best-effort; child may already be exiting
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
On cancel: `Drop` kills the child (SIGHUP); the child exits; the waiter
|
||||
thread's `wait()` reaps it and exits (its `oneshot::send` fails silently —
|
||||
the receiver was dropped with the future, which is expected); the
|
||||
reader/writer threads exit on channel close. The child is reaped (no zombie)
|
||||
by the waiter thread's `wait()` returning after the kill.
|
||||
|
||||
On happy path: the future resolves, the guard is disarmed (`Option::take()`
|
||||
in `poll`'s `Ready` branch), and the subsequent `Drop` is a no-op. The
|
||||
contract is "kill on cancel; no-op on resolve."
|
||||
|
||||
**Critical**: the POC's `LocalPty::exit_code` was a bare `oneshot::Receiver<i32>`
|
||||
with no kill guard. An implementer who copies the POC's shape without the
|
||||
guard violates the contract and will orphan processes on cancel. This task
|
||||
MUST add the guard.
|
||||
|
||||
### allocate_pty function
|
||||
|
||||
```rust
|
||||
pub fn allocate_pty(
|
||||
terminal: TerminalParams,
|
||||
cmd: Vec<String>,
|
||||
cwd: Option<PathBuf>,
|
||||
env: HashMap<String, String>,
|
||||
) -> Result<TtyHandle, TtyError>;
|
||||
```
|
||||
|
||||
This is called by `LocalTtyBackend::allocate()` (task `tty-local/backend-impl`)
|
||||
when `TtyParams.terminal` is `Some`. It returns a fully-wired `TtyHandle`.
|
||||
|
||||
### Tests
|
||||
|
||||
- **Happy path**: spawn `echo hello` into a PTY, read stdout until the
|
||||
zero-length sentinel, await exit_code → 0.
|
||||
- **Stdin round-trip**: spawn `cat` into a PTY, write bytes via the
|
||||
`AsyncWrite` stdin, read them back via stdout, send `Eof`, await exit 0.
|
||||
- **Resize**: call `PtyControl::resize`, assert no error (the ioctl succeeds).
|
||||
- **Signal (Unix)**: spawn `sleep 60`, send `signal("INT")`, assert the child
|
||||
exits with a signal-terminated status (negative exit code or 130 for SIGINT).
|
||||
Test process-group targeting: spawn `bash -c "sleep 60"`, send INT, assert
|
||||
the `sleep` child also receives it (the group signal).
|
||||
- **Cancel cleanup (ADR-056)**: spawn `sleep 60`, drop the `TtyHandle` (and
|
||||
thus the `exit_code` future) without awaiting it, assert the child is
|
||||
killed (reaped by the waiter thread; no orphan). Use a short delay then
|
||||
check the process is gone.
|
||||
- **Unknown signal name**: send `signal("NOSUCH")`, assert it falls back to
|
||||
`ChildKiller::kill` (SIGHUP); the child exits.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `allocate_pty` function spawns via `portable_pty::native_pty_system().openpty()`
|
||||
- [ ] `CommandBuilder::set_controlling_tty(true)` (the default; session leader)
|
||||
- [ ] Three std threads (reader, writer, waiter) feeding tokio mpsc/oneshot
|
||||
- [ ] `TtyHandle.stdout` is the reader mpsc wrapped as `Pin<Box<dyn Stream<Item = Bytes> + Send>>`
|
||||
- [ ] `TtyHandle.stdin` is the writer mpsc wrapped as `Box<dyn AsyncWrite + Send + Unpin>`
|
||||
- [ ] `TtyHandle.stderr` is `None` (PTY merges stdout/stderr)
|
||||
- [ ] `TtyHandle.exit_code` is `LocalExitFuture` (oneshot + kill guard), `BoxFuture<Result<i32, TtyError>>`
|
||||
- [ ] `TtyHandle.control` is `Some(TtyControlHandle::new(Arc::new(PtyControl)))`
|
||||
- [ ] `PtyControl` implements `alknet_tty::TtyControl` (resize, signal)
|
||||
- [ ] `PtyControl::signal` uses `kill(-pgid, sig)` with `kill(pid, sig)` fallback (Unix); `ChildKiller::kill` (non-Unix)
|
||||
- [ ] `PtyControl::signal` uses `alknet_tty::signal_from_name` for name→number
|
||||
- [ ] Unknown signal names fall back to `ChildKiller::kill` (SIGHUP)
|
||||
- [ ] `LocalExitFuture::Drop` calls `ChildKiller::kill(SIGHUP)` when dropped without resolving (ADR-056)
|
||||
- [ ] `LocalExitFuture` disarms the guard on resolve (no-op Drop on happy path)
|
||||
- [ ] Reader thread sends zero-length `Bytes` sentinel on EOF
|
||||
- [ ] Writer thread drops the writer on `StdinCmd::Eof`
|
||||
- [ ] Integration tests: happy path, stdin round-trip, resize, signal (process group), cancel cleanup, unknown signal
|
||||
- [ ] `cargo test -p alknet-tty-local` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty-local` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-local.md — §"PTY Mode", §"REQ-TTY-02", §"Cancel-Cleanup (ADR-056)"
|
||||
- docs/architecture/crates/tty/tty-backend.md — `TtyHandle`, `TtyControl` (the types this produces)
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md — ADR-053 (REQ-TTY-01)
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md — ADR-056 (kill-on-Drop)
|
||||
- /workspace/alknet-tty-poc/src/local_pty.rs — the reference implementation (port + add kill guard)
|
||||
- /workspace/alknet-tty-poc/tests/signal.rs — the SIGINT-forwarding integration test (validates REQ-TTY-02)
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the highest-risk task in the tty-local crate: the three-thread
|
||||
> bridge, the process-group signal targeting, and the ADR-056 kill guard are
|
||||
> all subtle. The POC is the reference but its `exit_code` was a bare
|
||||
> `oneshot::Receiver<i32>` — the kill guard is NEW and required. Do not copy
|
||||
> the POC's `exit_code` shape without the guard. The `AsyncWrite` wrapper over
|
||||
> the mpsc sender is also new (the POC exposed the sender directly); the
|
||||
> `TtyHandle.stdin` field requires `Box<dyn AsyncWrite + Send + Unpin>`.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,128 @@
|
||||
---
|
||||
id: tty-local/review-tty-local
|
||||
name: Review alknet-tty-local for REQ-TTY-01/02 and ADR-056 contract conformance
|
||||
status: pending
|
||||
depends_on: [tty-local/backend-impl]
|
||||
scope: moderate
|
||||
risk: low
|
||||
impact: phase
|
||||
level: review
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review the `alknet-tty-local` implementation for spec conformance, with
|
||||
particular attention to the three load-bearing requirements: REQ-TTY-01
|
||||
(backends need not be natively async), REQ-TTY-02 (signal forwarding to the
|
||||
process group), and ADR-056 (kill-on-Drop cancel-cleanup). This is the
|
||||
quality checkpoint at the end of the local backend phase, before the
|
||||
integration tests and the feature re-export wire the two crates together.
|
||||
|
||||
### Review Checklist
|
||||
|
||||
1. **PTY mode conformance** (tty-local.md §"PTY Mode"):
|
||||
- `allocate_pty` uses `portable_pty::native_pty_system().openpty()`
|
||||
- `CommandBuilder::set_controlling_tty(true)` (the default; session leader)
|
||||
- Three std threads (reader, writer, waiter) feeding tokio mpsc/oneshot
|
||||
- `TtyHandle.stdout` is the reader mpsc as `Pin<Box<dyn Stream<Item = Bytes> + Send>>`
|
||||
- `TtyHandle.stdin` is the writer mpsc as `Box<dyn AsyncWrite + Send + Unpin>`
|
||||
- `TtyHandle.stderr` is `None` (PTY merges stdout/stderr)
|
||||
- `TtyHandle.exit_code` is `LocalExitFuture` (oneshot + kill guard)
|
||||
- `TtyHandle.control` is `Some(TtyControlHandle::new(Arc::new(PtyControl)))`
|
||||
- Reader thread sends zero-length `Bytes` sentinel on EOF
|
||||
- Writer thread drops the writer on `StdinCmd::Eof`
|
||||
- `AsyncWrite` wrapper over the mpsc sender (poll_write, poll_flush, poll_close)
|
||||
|
||||
2. **REQ-TTY-02: Signal forwarding to the process group** (tty-local.md §"REQ-TTY-02"):
|
||||
- `PtyControl::signal` uses `kill(-pgid, sig)` (negative pid = process group)
|
||||
- Fallback to `kill(pid, sig)` if the group signal fails
|
||||
- `portable_pty` child is a session leader (`controlling_tty = true`)
|
||||
- `signal_from_name` from `alknet-tty` for the 9 supported names
|
||||
- Unknown names fall back to `ChildKiller::kill` (SIGHUP)
|
||||
- Contract is "best-effort delivery to the foreground process group"
|
||||
- Integration test validates process-group targeting (a child of the shell receives the signal)
|
||||
|
||||
3. **Pipe mode conformance** (tty-local.md §"Pipe Mode"):
|
||||
- `allocate_pipe` uses `tokio::process::Command` with `Stdio::piped()`
|
||||
- `TtyHandle.stdin` is `ChildStdin` as `Box<dyn AsyncWrite + Send + Unpin>`
|
||||
- `TtyHandle.stdout`/`stderr` wrapped as `Pin<Box<dyn Stream<Item = Bytes> + Send>>`
|
||||
- `TtyHandle.stderr` is `Some` (separate streams)
|
||||
- `TtyHandle.exit_code` is `PipeExitFuture` (wait + kill guard)
|
||||
- `TtyHandle.control` is `Some(TtyControlHandle::new(Arc::new(PipeControl)))`
|
||||
- `PipeControl::resize` is a no-op
|
||||
- `PipeControl::signal` uses `kill(pid, sig)` (NOT process group; documented limitation)
|
||||
- Unknown signal names fall back to `start_kill()` (SIGKILL)
|
||||
|
||||
4. **ADR-056: kill-on-Drop cancel-cleanup** (tty-local.md §"Cancel-Cleanup"):
|
||||
- `LocalExitFuture::Drop` calls `ChildKiller::kill(SIGHUP)` when dropped without resolving
|
||||
- `LocalExitFuture` disarms the guard on resolve (no-op Drop on happy path)
|
||||
- `PipeExitFuture::Drop` calls `Child::start_kill()` when dropped without resolving
|
||||
- `PipeExitFuture` disarms the guard on resolve
|
||||
- Neither future is a bare `oneshot::Receiver` / bare `Child::wait()` (the POC's shape — MUST have the guard)
|
||||
- Integration test: drop the `TtyHandle` mid-session, assert the child is killed (no orphan)
|
||||
|
||||
5. **LocalTtyBackend conformance** (tty-local.md, tty-backend.md):
|
||||
- `LocalTtyBackend::new()` takes no deps
|
||||
- `allocate()` branches on `params.terminal` (Some → PTY, None → pipe)
|
||||
- `resource_id()` returns `None`
|
||||
- Empty `cmd` → `TtyError::AllocFailed`
|
||||
- `backend_params` ignored (no backend-specific selector fields)
|
||||
|
||||
6. **Dependency constraints**:
|
||||
- `portable_pty` and `libc` deps present (the heavy deps, here not in alknet-tty)
|
||||
- `alknet-tty` dependency (workspace path) for the trait and types
|
||||
- No `alknet-core` direct dependency (accessed via alknet-tty if needed)
|
||||
- No `bollard`/`russh` (those are future backend crates)
|
||||
|
||||
7. **Pattern consistency**:
|
||||
- `TtyControl` impls use `alknet_tty::TtyControl` trait
|
||||
- `TtyControlHandle::new(Arc::new(...))` wrapping (OQ-43 pattern)
|
||||
- `TtyError` for errors (not `anyhow` — the POC used `anyhow`; the crate uses `TtyError`)
|
||||
- `tracing` for structured logging
|
||||
- `#[cfg(unix)]` gates on `libc::kill` paths
|
||||
|
||||
8. **Test coverage**:
|
||||
- PTY: happy path, stdin round-trip, resize, signal (process group), cancel cleanup, unknown signal
|
||||
- Pipe: happy path, stdin round-trip, separate stderr, signal, cancel cleanup, resize no-op, unknown signal
|
||||
- Backend: PTY dispatch, pipe dispatch, empty cmd, resource_id
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] PTY mode matches tty-local.md §"PTY Mode" (three-thread bridge, PtyControl)
|
||||
- [ ] REQ-TTY-02 satisfied: `kill(-pgid, sig)` with fallback, session leader, process-group test passes
|
||||
- [ ] Pipe mode matches tty-local.md §"Pipe Mode" (tokio::process, PipeControl, stderr Some)
|
||||
- [ ] ADR-056 satisfied: both `LocalExitFuture` and `PipeExitFuture` have kill-on-Drop guards
|
||||
- [ ] Neither exit future is a bare `oneshot::Receiver` / bare `Child::wait()` (the POC's shape)
|
||||
- [ ] `LocalTtyBackend` branches on `terminal`, `resource_id` returns `None`
|
||||
- [ ] `portable_pty`/`libc` deps present; no `bollard`/`russh`/`alknet-core` direct dep
|
||||
- [ ] `TtyControl` impls use the `alknet_tty::TtyControl` trait + `TtyControlHandle::new` wrapping
|
||||
- [ ] `#[cfg(unix)]` gates on `libc::kill` paths
|
||||
- [ ] `cargo fmt --check -p alknet-tty-local` passes
|
||||
- [ ] `cargo clippy -p alknet-tty-local` passes with no warnings
|
||||
- [ ] All tests pass (PTY + pipe + backend)
|
||||
- [ ] Cancel-cleanup integration tests confirm no orphaned processes
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-local.md — the authoritative spec
|
||||
- docs/architecture/crates/tty/tty-backend.md — the trait this implements
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md — ADR-053 (REQ-TTY-01)
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md — ADR-054
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md — ADR-056
|
||||
- /workspace/alknet-tty-poc/src/local_pty.rs — the reference (note: POC lacks the kill guard)
|
||||
- /workspace/alknet-tty-poc/tests/signal.rs — the SIGINT-forwarding test (REQ-TTY-02)
|
||||
|
||||
## Notes
|
||||
|
||||
> This review focuses on the three load-bearing requirements. The ADR-056
|
||||
> kill guard is the most likely deviation — the POC's `exit_code` was a bare
|
||||
> `oneshot::Receiver<i32>` without a guard, and an implementer who copies the
|
||||
> POC's shape verbatim violates the contract. Verify both `LocalExitFuture`
|
||||
> and `PipeExitFuture` have the guard and the cancel-cleanup tests pass (no
|
||||
> orphaned processes). REQ-TTY-02's process-group targeting is the other
|
||||
> subtle requirement — verify the integration test spawns a shell with a
|
||||
> child and confirms the child receives the signal.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,241 @@
|
||||
---
|
||||
id: tty/adapter
|
||||
name: Implement TtyAdapter (ProtocolHandler) and three-pump session driver
|
||||
status: pending
|
||||
depends_on: [tty/wire-codec, tty/control-messages, tty/negotiation, tty/backend-trait]
|
||||
scope: broad
|
||||
risk: high
|
||||
impact: phase
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the `TtyAdapter` (`ProtocolHandler` on `alknet/tty`) and the
|
||||
`drive_session` three-pump bidirectional driver in `src/adapter.rs`. This is
|
||||
where the wire format (ADR-052), the backend trait (ADR-053), and the
|
||||
exit-chunk ordering (ADR-055) come together. The adapter is backend-agnostic;
|
||||
the backends are wire-format-agnostic. The inversion is the `TtyBackend` trait.
|
||||
|
||||
This is a generalization of the POC's `/workspace/alknet-tty-poc/src/session.rs`,
|
||||
which hardcoded the local PTY backend; the adapter dispatches to any
|
||||
`TtyBackend`.
|
||||
|
||||
### TtyAdapter struct
|
||||
|
||||
```rust
|
||||
pub struct TtyAdapter {
|
||||
/// Backends keyed by the negotiation frame's `backend` string
|
||||
/// ("local", "docker", "ssh"). Populated at construction.
|
||||
backends: Arc<HashMap<String, Arc<dyn TtyBackend>>>,
|
||||
/// Optional ownership provider (ADR-050) for terminal sessions as
|
||||
/// runtime-spawned resources. None = no resource-level ACL (scope-
|
||||
/// gate only). Wired by the assembly layer.
|
||||
ownership: Option<Arc<dyn OwnershipProvider>>,
|
||||
}
|
||||
|
||||
impl TtyAdapter {
|
||||
pub fn new(backends: HashMap<String, Arc<dyn TtyBackend>>) -> Self;
|
||||
pub fn with_ownership(backends: HashMap<String, Arc<dyn TtyBackend>>, ownership: Arc<dyn OwnershipProvider>) -> Self;
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl ProtocolHandler for TtyAdapter {
|
||||
fn alpn(&self) -> &'static [u8] { b"alknet/tty" }
|
||||
|
||||
async fn handle(&self, connection: Connection, auth: &AuthContext)
|
||||
-> Result<(), HandlerError>
|
||||
{
|
||||
// One connection → many sessions (one bidi stream each).
|
||||
while let Ok((send, recv)) = connection.accept_bi().await {
|
||||
let backends = self.backends.clone();
|
||||
let ownership = self.ownership.clone();
|
||||
let identity = auth.identity.clone();
|
||||
tokio::spawn(async move {
|
||||
let _ = drive_session(send, recv, backends, ownership, identity).await;
|
||||
});
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Session lifecycle (drive_session)
|
||||
|
||||
A `alknet/tty` session on one bidi stream proceeds in three phases:
|
||||
|
||||
1. **Negotiation.** Read the single length-prefixed JSON negotiation frame
|
||||
(task `tty/negotiation`), parse into `NegotiateRequest`, extract the
|
||||
`backend` string, look up the `TtyBackend`, construct `TtyParams`. If the
|
||||
backend is not registered → `unknown_backend` error. If the frame is
|
||||
malformed or `carriage != "raw"` or `cmd` is empty → `malformed_negotiation`
|
||||
error. Send the JSON error response in negotiation framing and close the
|
||||
stream (do NOT enter raw mode).
|
||||
|
||||
2. **Allocation.** Call `backend.allocate(¶ms)`. If it fails →
|
||||
`allocate_failed` error response, close. Before allocating, run the access
|
||||
control checks (see below).
|
||||
|
||||
3. **Raw carriage — the bidirectional pump.** Switch to the raw chunk format
|
||||
and pump three concurrent tasks:
|
||||
|
||||
- **A. stdout → client**: backend stdout (`TtyHandle.stdout`) → stdout
|
||||
chunks (stream_type 1). If `TtyHandle.stderr` is `Some`, a concurrent
|
||||
stderr pump emits stderr chunks (stream_type 2). On backend stdout EOF,
|
||||
emit a zero-length stdout sentinel.
|
||||
- **B. client → backend**: client chunks → backend. stdin chunks
|
||||
(stream_type 0) → `TtyHandle.stdin` (via `AsyncWrite`). Control chunks
|
||||
(stream_type 3) → `ControlMessage` dispatch: `Resize` →
|
||||
`TtyControl::resize`, `Signal` → `TtyControl::signal`, `Eof` → close
|
||||
stdin. `Exit` from the client is ignored (server→client only). On client
|
||||
read-half close or a zero-length stdin chunk, signal EOF to the backend's
|
||||
stdin. Unknown control `type` values are ignored (logged at debug), not
|
||||
errors.
|
||||
- **C. exit → exit chunk**: await `TtyHandle.exit_code`; on resolve, enqueue
|
||||
`{"type":"exit","code":N}` as a control chunk (stream_type 3). On
|
||||
`TtyError`, send `{"type":"exit","code":-1}` (ADR-055 §4).
|
||||
|
||||
A drainer task writes chunks to the client in arrival order. After the exit
|
||||
chunk is written (task C resolves AND stdout/stderr pumps complete AND the
|
||||
exit chunk drains), the adapter closes the write half — the session ends.
|
||||
|
||||
### Exit-chunk ordering (ADR-055)
|
||||
|
||||
The "exit chunk is last" invariant is enforced here, in the adapter's session
|
||||
driver, not in the backend:
|
||||
|
||||
1. The stdout pump (task A) drains the backend's stdout to EOF.
|
||||
2. The exit task (task C) awaits `TtyHandle.exit_code`.
|
||||
3. **The adapter waits for *both* the stdout pump to complete (EOF) *and*
|
||||
`exit_code` to resolve** before enqueueing the exit chunk. If `stderr` is
|
||||
`Some`, it also drains before the exit chunk. (ADR-055 assumption 2.)
|
||||
4. After both resolve, the exit chunk is enqueued on the writer channel.
|
||||
5. The drainer writes the exit chunk to the client.
|
||||
6. The adapter closes the write half — the session ends.
|
||||
|
||||
Use a coordination primitive (e.g., `tokio::join!` on the stdout pump and the
|
||||
exit future, or a barrier) to enforce the "both done before exit chunk" rule.
|
||||
The POC's `session.rs` uses an mpsc writer channel where the exit task sends
|
||||
the exit chunk and the drainer writes it last; the adapter must additionally
|
||||
ensure the stdout pump has finished before the exit chunk is sent. A clean
|
||||
pattern: `join!(stdout_pump, exit_future)` then send the exit chunk.
|
||||
|
||||
### Access control
|
||||
|
||||
Terminal sessions are runtime-spawned resources per ADR-050:
|
||||
|
||||
- **Scope-gate at negotiation.** Check the caller's `identity.scopes` for the
|
||||
`tty:open` scope (or a deployment-configured scope) before allocating. A
|
||||
caller without the scope gets `{"error":"forbidden"}` and the stream closes.
|
||||
The scope name is a two-way-door choice (reversible, not a wire-format
|
||||
constant).
|
||||
- **Resource ownership for backend-specific resources.** Call
|
||||
`backend.resource_id(¶ms)`. If `Some((kind, id))` and an ownership
|
||||
provider is wired, check `OwnershipProvider::owns(identity, kind, id, "tty")`.
|
||||
If the caller doesn't own it → `{"error":"forbidden"}`. If `None` (the
|
||||
session creates its own resource — local process, SSH channel), no
|
||||
ownership check.
|
||||
- **`forwarded_for`** for proxied sessions (ADR-032) is the hub's concern, not
|
||||
the adapter's — the worker authorizes the hub (its direct caller).
|
||||
|
||||
### Negotiation errors
|
||||
|
||||
Send the JSON error response in negotiation framing (task `tty/negotiation`),
|
||||
then close the write half. The framing-disambiguation trick (first byte `0x00`
|
||||
= error frame, first byte `1`/`2`/`3` = raw chunk) is sound because error frames
|
||||
are under 16 MiB.
|
||||
|
||||
### Connection and stream lifecycle
|
||||
|
||||
- **Connection drop**: all in-flight sessions are cancelled. Pump tasks drop;
|
||||
`TtyHandle` drops; `exit_code` future drops without completion → backend's
|
||||
cancel-cleanup kills the session target (ADR-056).
|
||||
- **Stream reset**: `ChunkReader` returns `RawError` (ConnectionClosed or Io).
|
||||
Pump tasks exit; `TtyHandle` drops; cancel-cleanup runs. No exit chunk sent.
|
||||
- **Client cancel (write-half close / eof / zero-length stdin)**: signal EOF
|
||||
to backend stdin, keep pumping stdout until exit resolves. The session
|
||||
completes normally (exit chunk sent). This is NOT a cancel — ADR-056 is not
|
||||
triggered. Cancel-cleanup is triggered only when the *adapter* drops the
|
||||
handle (connection drop, stream reset, panic).
|
||||
|
||||
### Tests
|
||||
|
||||
Use the `MockBackend` from task `tty/backend-trait` (in-memory tokio mpsc
|
||||
channels for stdin/stdout, a oneshot for exit_code, a mock `TtyControl`).
|
||||
Drive `drive_session` over a `tokio::io::duplex`:
|
||||
|
||||
- **Happy path**: send a negotiation frame, send stdin chunks, mock backend
|
||||
echoes stdout, resolve exit_code 0, assert the client receives stdout
|
||||
chunks then the exit chunk last, then stream close.
|
||||
- **Exit-chunk-is-last**: assert no stdout chunk arrives after the exit chunk.
|
||||
- **Stdin EOF**: send a zero-length stdin chunk (or `eof` control), assert the
|
||||
backend's stdin closes, stdout continues, exit chunk still sent.
|
||||
- **Resize/signal control**: send resize and signal control chunks, assert
|
||||
the mock `TtyControl` receives them.
|
||||
- **Unknown control type**: send `{"type":"unknown"}`, assert it's ignored
|
||||
(no error, session continues).
|
||||
- **unknown_backend error**: negotiation frame with unregistered backend →
|
||||
error response, stream closes, no raw mode.
|
||||
- **malformed_negotiation error**: bad JSON or `carriage != "raw"` or empty
|
||||
`cmd` → error response, stream closes.
|
||||
- **allocate_failed error**: mock backend returns `TtyError::AllocFailed` →
|
||||
error response, stream closes.
|
||||
- **Exit error**: mock backend's `exit_code` resolves with `TtyError` →
|
||||
`{"type":"exit","code":-1}`.
|
||||
- **Cancel cleanup**: drop the connection mid-session, assert the mock
|
||||
backend's `exit_code` future is dropped (the kill-on-Drop guard fires).
|
||||
- **Scope gate**: identity without `tty:open` scope → `forbidden` error.
|
||||
- **Ownership check**: backend returns `resource_id Some`, ownership provider
|
||||
returns false → `forbidden`; returns true → session proceeds.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `TtyAdapter` struct with `backends` and `ownership` fields
|
||||
- [ ] `TtyAdapter::new` and `with_ownership` constructors
|
||||
- [ ] `impl ProtocolHandler for TtyAdapter` with `alpn()` returning `b"alknet/tty"`
|
||||
- [ ] `handle()` loops `accept_bi`, spawns `drive_session` per stream
|
||||
- [ ] `drive_session` reads negotiation frame, parses, selects backend, constructs `TtyParams`
|
||||
- [ ] Negotiation errors (`unknown_backend`, `malformed_negotiation`, `allocate_failed`) sent as JSON in negotiation framing, stream closed
|
||||
- [ ] Three-pump driver: stdout→client, client→backend (stdin + control dispatch), exit→exit-chunk
|
||||
- [ ] stderr pump concurrent with stdout when `TtyHandle.stderr` is `Some`
|
||||
- [ ] Exit-chunk-is-last invariant: stdout/stderr pumps complete AND exit_code resolves before exit chunk enqueued
|
||||
- [ ] Exit error → `{"type":"exit","code":-1}`
|
||||
- [ ] Unknown control `type` ignored (not an error)
|
||||
- [ ] `Exit` control from client ignored (server→client only)
|
||||
- [ ] Zero-length stdin chunk and `eof` control both close backend stdin
|
||||
- [ ] Scope-gate at negotiation (`tty:open` scope, or deployment-configured)
|
||||
- [ ] Ownership check via `backend.resource_id()` + `OwnershipProvider::owns()` when wired
|
||||
- [ ] Connection drop / stream reset drops `TtyHandle` (triggers ADR-056 cancel-cleanup)
|
||||
- [ ] Client write-half close does NOT trigger cancel-cleanup (session runs to completion)
|
||||
- [ ] Integration tests with `MockBackend` over `tokio::io::duplex` for all the scenarios above
|
||||
- [ ] `cargo test -p alknet-tty` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-adapter.md — the authoritative session lifecycle spec
|
||||
- docs/architecture/crates/tty/tty-wire.md — the wire format the adapter pumps
|
||||
- docs/architecture/crates/tty/tty-backend.md — the backend trait the adapter dispatches to
|
||||
- docs/architecture/decisions/052-alknet-tty-wire-format-and-two-carriage.md — ADR-052
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md — ADR-053
|
||||
- docs/architecture/decisions/055-exit-code-on-control-chunk.md — ADR-055 (exit-chunk ordering)
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md — ADR-056 (cancel-cleanup)
|
||||
- docs/architecture/decisions/050-dynamic-resource-ownership-for-runtime-spawned-resources.md — ADR-050 (access control)
|
||||
- docs/architecture/decisions/032-forwarded-for-identity.md — ADR-032 (forwarded_for)
|
||||
- /workspace/alknet-tty-poc/src/session.rs — the reference three-pump driver (hardcoded to local PTY; generalize to the trait)
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the integration task where all the invariants come together. The
|
||||
> exit-chunk-is-last ordering (ADR-055) is the subtle part: the adapter must
|
||||
> wait for BOTH the stdout pump to complete AND exit_code to resolve before
|
||||
> sending the exit chunk. The POC's `session.rs` is the reference but it does
|
||||
> not enforce this ordering strictly (the exit task sends the chunk
|
||||
> independently); the crate's adapter must add the coordination. The
|
||||
> `MockBackend` from `tty/backend-trait` is the test fixture. The scope name
|
||||
> `tty:open` is a two-way-door choice — make it a constant or configurable,
|
||||
> not a wire-format constant.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,239 @@
|
||||
---
|
||||
id: tty/backend-trait
|
||||
name: Implement TtyBackend trait, TtyHandle, TtyControl, TtyParams, TtyError (ADR-053)
|
||||
status: pending
|
||||
depends_on: [tty/crate-init]
|
||||
scope: moderate
|
||||
risk: high
|
||||
impact: phase
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the `TtyBackend` trait and its associated types in `src/backend.rs`.
|
||||
This is the **inversion point** (ADR-053) between the wire-format adapter and
|
||||
the backends. alknet-tty defines the trait; the backend crates
|
||||
(`alknet-tty-local`, future `alknet-docker`, `alknet-ssh`) implement it. The
|
||||
trait shape is a **one-way door** (ADR-053) — changing it after backends exist
|
||||
is a rewrite across crates. Get it right.
|
||||
|
||||
### TtyBackend trait
|
||||
|
||||
```rust
|
||||
#[async_trait]
|
||||
pub trait TtyBackend: Send + Sync {
|
||||
/// Allocate a terminal/process session and return the handles the
|
||||
/// adapter pumps. The `backend` field of the negotiation frame
|
||||
/// (ADR-052) selects which registered backend's `allocate` is called.
|
||||
async fn allocate(&self, params: &TtyParams) -> Result<TtyHandle, TtyError>;
|
||||
|
||||
/// The pre-existing resource this session targets, for ownership
|
||||
/// checks (ADR-050). `None` = no pre-existing resource (the session
|
||||
/// creates its own — local process, SSH channel). `Some((kind, id))`
|
||||
/// = the session targets an existing resource the caller must own
|
||||
/// (e.g., DockerTtyBackend returns `Some(("container", id))`). The
|
||||
/// adapter calls this at negotiation to gate access; the backend
|
||||
/// extracts the id from its own `backend_params`. Default `None`.
|
||||
fn resource_id(&self, _params: &TtyParams) -> Option<(&'static str, String)> { None }
|
||||
}
|
||||
```
|
||||
|
||||
The adapter holds `HashMap<String, Arc<dyn TtyBackend>>` populated at
|
||||
construction. A backend is the *thing that allocates a session*; the
|
||||
wire-format pump is backend-agnostic.
|
||||
|
||||
### TtyError
|
||||
|
||||
`#[non_exhaustive]` so new variants are additive (two-way-door extension
|
||||
within the one-way trait shape — ADR-053).
|
||||
|
||||
```rust
|
||||
#[non_exhaustive]
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum TtyError {
|
||||
#[error("allocate failed: {message}")]
|
||||
AllocFailed { message: String },
|
||||
#[error("wait failed: {message}")]
|
||||
WaitFailed { message: String },
|
||||
#[error("io: {0}")]
|
||||
Io(#[from] std::io::Error),
|
||||
#[error("backend-specific: {message}")]
|
||||
Backend { message: String },
|
||||
}
|
||||
```
|
||||
|
||||
- `AllocFailed` — PTY couldn't be allocated, docker exec failed, SSH channel
|
||||
rejected. Returned by `allocate()`; adapter sends `allocate_failed` error.
|
||||
- `WaitFailed` — backend couldn't reap the child / determine exit code.
|
||||
Returned by the `exit_code` future; adapter sends `{"type":"exit","code":-1}`.
|
||||
- `Io` — I/O error from a backend's stream/handle.
|
||||
- `Backend` — backend-specific error not covered by the above.
|
||||
|
||||
### TtyParams — the allocation request
|
||||
|
||||
```rust
|
||||
pub struct TtyParams {
|
||||
/// Terminal parameters. `None` = pipe mode (no PTY — the runner case,
|
||||
/// ADR-054). `Some` = allocate a PTY with these dimensions.
|
||||
pub terminal: Option<TerminalParams>,
|
||||
/// Command vector (argv[0] + args). Non-empty.
|
||||
pub cmd: Vec<String>,
|
||||
/// Working directory (None = inherit/default).
|
||||
pub cwd: Option<PathBuf>,
|
||||
/// Environment variables (empty = inherit).
|
||||
pub env: HashMap<String, String>,
|
||||
/// Backend-specific selector fields from the negotiation frame,
|
||||
/// unparsed. The adapter passes the JSON object through verbatim; the
|
||||
/// backend deserializes its own strongly-typed params struct from it.
|
||||
/// alknet-tty has zero knowledge of any backend's params shape.
|
||||
pub backend_params: serde_json::Map<String, serde_json::Value>,
|
||||
}
|
||||
|
||||
pub struct TerminalParams {
|
||||
pub term: Option<String>, // e.g., "xterm-256color"; None = backend default
|
||||
pub cols: u16,
|
||||
pub rows: u16,
|
||||
pub pixel_width: u16,
|
||||
pub pixel_height: u16,
|
||||
pub modes: serde_json::Value, // reserved — OQ-44; backends MUST ignore content in v1
|
||||
}
|
||||
```
|
||||
|
||||
Provide a conversion `From<NegotiateRequest> for TtyParams` (or a constructor)
|
||||
that the adapter uses — mapping `NegotiateRequest.tty: Option<TerminalParamsWire>`
|
||||
to `TtyParams.terminal: Option<TerminalParams>`, and passing `backend_params`
|
||||
through verbatim. This conversion lives here (in `backend.rs` or
|
||||
`negotiation.rs`) so the adapter doesn't hand-roll it.
|
||||
|
||||
### TtyHandle — what a backend produces
|
||||
|
||||
```rust
|
||||
pub struct TtyHandle {
|
||||
/// Stdin writer — bytes the adapter pumps from client stdin chunks.
|
||||
/// `tokio::io::AsyncWrite` (the tokio flavor, not `futures::io`).
|
||||
pub stdin: Box<dyn tokio::io::AsyncWrite + Send + Unpin>,
|
||||
/// Stdout stream — bytes the adapter pumps to client stdout chunks.
|
||||
/// Ends when the backend's stdout reaches EOF.
|
||||
pub stdout: Pin<Box<dyn futures_core::Stream<Item = bytes::Bytes> + Send>>,
|
||||
/// Stderr stream — `None` for PTY backends (stdout/stderr merged
|
||||
/// into `stdout`). `Some` for pipe backends (separate streams).
|
||||
pub stderr: Option<Pin<Box<dyn futures_core::Stream<Item = bytes::Bytes> + Send>>>,
|
||||
/// Exit code — a `Future` the adapter awaits. Resolves when the
|
||||
/// process/container/SSH exec exits. The adapter sends the result
|
||||
/// as the `{"type":"exit","code":N}` control chunk (ADR-055) and
|
||||
/// closes the stream. This is `BoxFuture`, not a method on
|
||||
/// `TtyHandle`, so the adapter can `select` between exit and
|
||||
/// stream-close without coupling to the other fields. (REQ-TTY-01.)
|
||||
pub exit_code: BoxFuture<'static, Result<i32, TtyError>>,
|
||||
/// Control handle (resize, signal) — `Clone` so the adapter can
|
||||
/// hand it to the spawned control-chunk dispatcher. `None` only
|
||||
/// when the backend genuinely has no control path. See OQ-43.
|
||||
pub control: Option<TtyControlHandle>,
|
||||
}
|
||||
```
|
||||
|
||||
### TtyControl trait and TtyControlHandle
|
||||
|
||||
```rust
|
||||
pub trait TtyControl: Send + Sync {
|
||||
/// Resize the terminal. Maps to SSH `window-change`, docker exec
|
||||
/// resize, or `ioctl(TIOCSWINSZ)` on a local PTY. No-op for pipe
|
||||
/// backends without a PTY.
|
||||
fn resize(&self, cols: u16, rows: u16, pixel_width: u16, pixel_height: u16);
|
||||
|
||||
/// Forward a signal by name. Best-effort delivery to the foreground
|
||||
/// process group (see tty-local.md REQ-TTY-02). Unknown names fall
|
||||
/// back to the backend's default kill.
|
||||
fn signal(&self, name: &str);
|
||||
}
|
||||
|
||||
/// The `Clone`-able handle to a backend's control path. The `TtyControl`
|
||||
/// trait is NOT `Clone` (`Clone` is not object-safe); the `Clone`-ability
|
||||
/// lives on this concrete newtype, which holds the trait object behind an
|
||||
/// `Arc`. See OQ-43.
|
||||
#[derive(Clone)]
|
||||
pub struct TtyControlHandle(Arc<dyn TtyControl + Send + Sync>);
|
||||
|
||||
impl TtyControlHandle {
|
||||
pub fn new(control: Arc<dyn TtyControl + Send + Sync>) -> Self { Self(control) }
|
||||
pub fn resize(&self, c: u16, r: u16, pw: u16, ph: u16) { self.0.resize(c, r, pw, ph) }
|
||||
pub fn signal(&self, name: &str) { self.0.signal(name) }
|
||||
}
|
||||
```
|
||||
|
||||
The trait is kept object-safe by NOT putting `Clone` on it; the `Clone` newtype
|
||||
(`TtyControlHandle`) holds the trait object behind an `Arc`. A backend produces
|
||||
its own control type via `TtyControlHandle::new(Arc::new(MyControl))` without
|
||||
the adapter knowing the concrete shape (OQ-43).
|
||||
|
||||
### REQ-TTY-01: backends are not required to be natively async
|
||||
|
||||
The trait's adapter-facing types (`AsyncWrite`, `Stream<Item = Bytes>`,
|
||||
`BoxFuture`, `TtyControl`) are the **adapter's contract**. A backend may expose
|
||||
blocking handles internally and bridge them to these async-facing types via
|
||||
std threads + tokio mpsc/oneshot (the pattern `portable_pty` requires). This is
|
||||
a documented, supported implementation strategy, not a workaround. The local
|
||||
backend (task `tty-local/pty-mode`) is the reference implementation.
|
||||
|
||||
### ADR-056: kill-on-Drop contract
|
||||
|
||||
Dropping the `exit_code` future MUST kill the session target. This is a
|
||||
behavioral contract on the `TtyBackend` trait — the adapter triggers it by
|
||||
dropping the `TtyHandle` on session cancel (connection drop, stream reset); the
|
||||
backend wires the kill into the `exit_code` future's `Drop`. A backend that
|
||||
returns a bare `oneshot::Receiver<i32>` (or any future without a kill-on-`Drop`
|
||||
guard) as `exit_code` violates the contract and will orphan processes on cancel.
|
||||
Document this contract in the `TtyHandle.exit_code` doc comment.
|
||||
|
||||
### Tests
|
||||
|
||||
This task defines types and traits; the concrete implementations are in the
|
||||
local backend tasks. Tests here are structural:
|
||||
- A mock backend (in-memory pipes) implementing `TtyBackend` for the adapter's
|
||||
tests is built in task `tty/adapter`. Here, write a minimal compile-time
|
||||
check: a `MockBackend` struct that implements `TtyBackend` returning a
|
||||
`TtyHandle` with tokio mpsc channels, to verify the trait is implementable
|
||||
and the types compose. This mock is reused by the adapter task.
|
||||
- `TtyControlHandle::new` + `Clone` + `resize`/`signal` delegation test with a
|
||||
mock `TtyControl`.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `TtyBackend` trait with `allocate()` (async) and `resource_id()` (default `None`)
|
||||
- [ ] `TtyError` enum, `#[non_exhaustive]`, with `AllocFailed`, `WaitFailed`, `Io`, `Backend`
|
||||
- [ ] `TtyParams` struct with `terminal`, `cmd`, `cwd`, `env`, `backend_params`
|
||||
- [ ] `TerminalParams` struct with `term`, `cols`, `rows`, `pixel_width`, `pixel_height`, `modes`
|
||||
- [ ] `TtyHandle` struct with `stdin` (Box<dyn AsyncWrite + Send + Unpin>), `stdout` (Pin<Box<dyn Stream<Item=Bytes> + Send>>), `stderr` (Option), `exit_code` (BoxFuture<Result<i32, TtyError>>), `control` (Option<TtyControlHandle>)
|
||||
- [ ] `TtyControl` trait with `resize` and `signal` (object-safe, NOT `Clone`)
|
||||
- [ ] `TtyControlHandle` newtype, `#[derive(Clone)]`, wraps `Arc<dyn TtyControl + Send + Sync>`, with `new`/`resize`/`signal`
|
||||
- [ ] `From<NegotiateRequest> for TtyParams` (or constructor) mapping wire types to params
|
||||
- [ ] `TtyHandle.exit_code` doc comment documents the ADR-056 kill-on-Drop contract
|
||||
- [ ] `TtyBackend` trait doc comment documents REQ-TTY-01 (backends need not be natively async)
|
||||
- [ ] A `MockBackend` (in-memory pipes) implements `TtyBackend` and compiles
|
||||
- [ ] Unit test: `TtyControlHandle::new` + clone + resize/signal delegation
|
||||
- [ ] `cargo test -p alknet-tty` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-backend.md — full trait spec (the authoritative source)
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md — ADR-053 (the trait decision)
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md — ADR-056 (kill-on-Drop contract)
|
||||
- docs/architecture/decisions/050-dynamic-resource-ownership-for-runtime-spawned-resources.md — ADR-050 (`resource_id`)
|
||||
- /workspace/alknet-tty-poc/src/local_pty.rs — `LocalPty` (the reference shape a backend produces)
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the one-way-door task (ADR-053). The trait shape, `TtyHandle` field
|
||||
> set, and `TtyControl` trait are the API surface every backend crate
|
||||
> implements and the adapter consumes. Review carefully before the local
|
||||
> backend begins. The `MockBackend` built here is reused by the adapter task
|
||||
> for the three-pump driver tests. The `BoxFuture` type comes from
|
||||
> `futures::future::BoxFuture` (or `std::pin::Pin<Box<dyn Future + Send>>`);
|
||||
> the `Stream` type from `futures_core::Stream`. Both are re-exported by
|
||||
> `tokio_stream` for extension methods.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,125 @@
|
||||
---
|
||||
id: tty/control-messages
|
||||
name: Implement ControlMessage enum and signal_from_name helper
|
||||
status: pending
|
||||
depends_on: [tty/crate-init]
|
||||
scope: single
|
||||
risk: low
|
||||
impact: isolated
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the control channel schema in `src/control.rs`. Control chunks
|
||||
(stream_type 3) carry a JSON payload tagged by `type`. This is a direct port of
|
||||
the POC's `/workspace/alknet-tty-poc/src/control.rs`.
|
||||
|
||||
### ControlMessage enum
|
||||
|
||||
```rust
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
#[serde(tag = "type", rename_all = "snake_case")]
|
||||
pub enum ControlMessage {
|
||||
Resize {
|
||||
cols: u16,
|
||||
rows: u16,
|
||||
#[serde(default)]
|
||||
pixel_width: u16,
|
||||
#[serde(default)]
|
||||
pixel_height: u16,
|
||||
},
|
||||
Signal { name: String },
|
||||
Eof,
|
||||
Exit { code: i32 },
|
||||
}
|
||||
```
|
||||
|
||||
| Direction | Message | Shape | Maps to |
|
||||
|---|---|---|---|
|
||||
| client→server | resize | `{"type":"resize","cols":80,"rows":24,...}` | SSH `window-change`, docker exec resize, `ioctl(TIOCSWINSZ)` |
|
||||
| client→server | signal | `{"type":"signal","name":"INT"}` | SSH `signal`, docker exec signal, `kill(-pgid, sig)` (REQ-TTY-02) |
|
||||
| client→server | eof | `{"type":"eof"}` | SSH channel EOF, docker stdin close, `ChildStdin::drop` |
|
||||
| server→client | exit | `{"type":"exit","code":0}` | the terminal/completion signal (ADR-055) |
|
||||
|
||||
### Serialization helpers
|
||||
|
||||
```rust
|
||||
impl ControlMessage {
|
||||
pub fn to_json(&self) -> serde_json::Result<bytes::Bytes>;
|
||||
pub fn from_slice(b: &[u8]) -> serde_json::Result<Self>;
|
||||
}
|
||||
```
|
||||
|
||||
### signal_from_name (Unix-only)
|
||||
|
||||
```rust
|
||||
#[cfg(unix)]
|
||||
pub fn signal_from_name(name: &str) -> Option<i32>;
|
||||
```
|
||||
|
||||
Maps uppercase signal names to `libc` signal numbers: `HUP`, `INT`, `QUIT`,
|
||||
`TERM`, `KILL`, `USR1`, `USR2`, `TSTP`, `CONT`. Unknown names return `None`;
|
||||
the caller (the local backend) decides whether to ignore or fall back to the
|
||||
backend's default kill. This helper lives in `alknet-tty` (not
|
||||
`alknet-tty-local`) because the wire spec defines the supported name set
|
||||
(tty-wire.md §"Control Channel") and the local backend consumes it.
|
||||
|
||||
On non-Unix targets, `signal_from_name` is absent (the local backend's
|
||||
non-Unix signal path falls back to `ChildKiller::kill` directly). The
|
||||
`#[cfg(unix)]` gate matches the POC.
|
||||
|
||||
### Extensibility
|
||||
|
||||
Unknown `type` values are **ignored** (not a protocol error) so that a newer
|
||||
client sending a control message an older server doesn't recognize degrades
|
||||
gracefully rather than tearing down the session. This is handled in the
|
||||
adapter's control dispatch (task `tty/adapter`), but the `Deserialize` impl
|
||||
must tolerate it — use `#[serde(other)]` on a catch-all variant OR handle the
|
||||
`serde_json::Error` in the adapter as "ignore unknown." The POC handles it in
|
||||
the adapter (logs a warning on parse error, continues). Match the POC's
|
||||
approach: do NOT add a catch-all variant to the enum; let `from_slice` return
|
||||
an error on unknown `type` and have the adapter ignore the error. This keeps
|
||||
the enum exhaustive and the wire format's "unknown types are ignored" rule an
|
||||
adapter-level policy, not a schema-level leak.
|
||||
|
||||
### Tests
|
||||
|
||||
- Round-trip: serialize each variant, deserialize, assert equality.
|
||||
- `to_json` produces the expected `{"type":"resize",...}` shape (snake_case tag).
|
||||
- `signal_from_name` returns the right numbers for all 9 names, `None` for
|
||||
unknown names (Unix only).
|
||||
- `from_slice` on `{"type":"unknown"}` returns an error (the adapter will
|
||||
ignore it).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `ControlMessage` enum with `Resize`, `Signal`, `Eof`, `Exit` variants
|
||||
- [ ] `#[serde(tag = "type", rename_all = "snake_case")]` attribute
|
||||
- [ ] `Resize` has `cols`, `rows`, `pixel_width` (default 0), `pixel_height` (default 0)
|
||||
- [ ] `Exit` has `code: i32`
|
||||
- [ ] `to_json` and `from_slice` helpers implemented
|
||||
- [ ] `signal_from_name` implemented for the 9 supported names, `#[cfg(unix)]` gated
|
||||
- [ ] Round-trip unit tests for all 4 variants
|
||||
- [ ] Unit test: `to_json` produces snake_case `type` tag
|
||||
- [ ] Unit test: `signal_from_name` for all 9 names + unknown (Unix only)
|
||||
- [ ] Unit test: `from_slice` on unknown `type` returns error
|
||||
- [ ] `cargo test -p alknet-tty` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-wire.md — §"Control Channel (stream_type 3)", §"Stdin Closure"
|
||||
- docs/architecture/decisions/055-exit-code-on-control-chunk.md — ADR-055 (Exit variant)
|
||||
- /workspace/alknet-tty-poc/src/control.rs — the reference implementation to port
|
||||
|
||||
## Notes
|
||||
|
||||
> Near-verbatim port of the POC's `control.rs`. The `signal_from_name` helper
|
||||
> is Unix-only; the local backend's non-Unix path uses `ChildKiller::kill`
|
||||
> directly. The "unknown type ignored" policy is enforced in the adapter, not
|
||||
> the schema — keep the enum exhaustive.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,133 @@
|
||||
---
|
||||
id: tty/crate-init
|
||||
name: Initialize alknet-tty crate with Cargo.toml, dependencies, and module skeleton
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: moderate
|
||||
risk: low
|
||||
impact: project
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Initialize the `alknet-tty` crate from scratch. This is the lean core crate for
|
||||
the `alknet/tty` ALPN: the wire format, the `TtyBackend` trait, and the
|
||||
`TtyAdapter` (`ProtocolHandler`). It depends on `alknet-core` only — no
|
||||
`portable_pty`, no `bollard`, no `russh`, no `alknet-call` (ADR-057). The local
|
||||
backend lives in a sibling crate (`alknet-tty-local`, ADR-054) behind a `local`
|
||||
feature re-export.
|
||||
|
||||
### Crate setup
|
||||
|
||||
Create `crates/alknet-tty/` with:
|
||||
|
||||
- `Cargo.toml` — package metadata, dependencies, feature flags
|
||||
- `src/lib.rs` — crate root with module declarations and re-exports
|
||||
- Module skeleton files (empty or with `// TODO` markers) for:
|
||||
- `src/wire.rs` — `ChunkReader`/`ChunkWriter`, `Chunk`, `RawError`, stream-type
|
||||
constants, `MAX_CHUNK_LEN`
|
||||
- `src/control.rs` — `ControlMessage` tagged enum, `signal_from_name`
|
||||
- `src/negotiation.rs` — `NegotiateRequest`, `TerminalParamsWire`,
|
||||
length-prefixed framing reader/writer, error response shape
|
||||
- `src/backend.rs` — `TtyBackend` trait, `TtyHandle`, `TtyControl`,
|
||||
`TtyControlHandle`, `TtyParams`, `TerminalParams`, `TtyError`
|
||||
- `src/adapter.rs` — `TtyAdapter` (`ProtocolHandler` on `alknet/tty`),
|
||||
`drive_session` three-pump driver
|
||||
|
||||
### Dependencies
|
||||
|
||||
Per the architecture specs (overview.md, tty-wire.md, tty-backend.md):
|
||||
|
||||
| Crate | Purpose |
|
||||
|-------|---------|
|
||||
| `alknet-core` | `ProtocolHandler`, `Connection`, `AuthContext`, `Identity`, `HandlerError`, `OwnershipProvider` (workspace path) |
|
||||
| `tokio` 1 (full) | Async runtime, mpsc/oneshot channels, `AsyncRead`/`AsyncWrite` |
|
||||
| `bytes` 1 | `Bytes` for chunk payloads and stdout streams |
|
||||
| `futures-core` | `Stream` trait for `TtyHandle.stdout`/`stderr` |
|
||||
| `tokio-stream` | `StreamExt` re-export of `futures_core::Stream` for extension methods |
|
||||
| `serde` 1 | Serialization for `NegotiateRequest`, `ControlMessage` |
|
||||
| `serde_json` 1 | JSON for negotiation frame, control channel, `backend_params` map |
|
||||
| `async-trait` 0.1 | `TtyBackend` trait (async fn in trait) |
|
||||
| `tracing` 0.1 | Structured logging |
|
||||
| `thiserror` 2 | Error enums (`TtyError`, `RawError`) |
|
||||
|
||||
No `portable_pty`, no `bollard`, no `russh`, no `alknet-call`. The negotiation
|
||||
framing is self-contained (~30 lines, ADR-057).
|
||||
|
||||
### Feature flags
|
||||
|
||||
```toml
|
||||
[features]
|
||||
default = []
|
||||
local = ["dep:alknet-tty-local"] # re-export LocalTtyBackend from alknet-tty-local
|
||||
```
|
||||
|
||||
- `default` — the wire format, `TtyAdapter`, and the `TtyBackend` trait. No
|
||||
backend implementations; the assembly layer registers backends from their own
|
||||
crates.
|
||||
- `local` — re-export `alknet_tty_local::LocalTtyBackend` as
|
||||
`alknet_tty::local::LocalTtyBackend`. Pulls in `alknet-tty-local` (which pulls
|
||||
in `portable_pty`). Wired in task `tty/local-feature-reexport`.
|
||||
|
||||
### Workspace Cargo.toml
|
||||
|
||||
Add `crates/alknet-tty` to the workspace `members` list in the root `Cargo.toml`.
|
||||
|
||||
### Module skeleton
|
||||
|
||||
```rust
|
||||
// src/lib.rs
|
||||
//! alknet-tty: Terminal session protocol handler for the `alknet/tty` ALPN.
|
||||
//!
|
||||
//! Two-carriage model (ADR-052): a JSON negotiation frame, then raw chunks
|
||||
//! (`[stream_type: u8][length: u32 be][payload]`). Backend-agnostic via the
|
||||
//! `TtyBackend` trait (ADR-053). Depends on alknet-core only (ADR-057).
|
||||
|
||||
pub mod wire;
|
||||
pub mod control;
|
||||
pub mod negotiation;
|
||||
pub mod backend;
|
||||
pub mod adapter;
|
||||
|
||||
// Re-exports filled in by subsequent tasks.
|
||||
```
|
||||
|
||||
Each module file gets a doc comment and `// TODO: implement` marker. The
|
||||
subsequent tasks (wire-codec, control-messages, negotiation, backend-trait,
|
||||
adapter) fill these in.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `crates/alknet-tty/Cargo.toml` exists with all dependencies and the `local` feature gate
|
||||
- [ ] `crates/alknet-tty/src/lib.rs` exists with module declarations
|
||||
- [ ] Module skeleton files exist: `wire.rs`, `control.rs`, `negotiation.rs`, `backend.rs`, `adapter.rs`
|
||||
- [ ] Root `Cargo.toml` `members` list includes `crates/alknet-tty`
|
||||
- [ ] `cargo check -p alknet-tty` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty` succeeds with no warnings
|
||||
- [ ] Dual licensing: `MIT OR Apache-2.0` (workspace-inherited)
|
||||
- [ ] `alknet-core` dependency uses workspace path (`path = "../alknet-core"`)
|
||||
- [ ] No `portable_pty`, `bollard`, `russh`, or `alknet-call` dependency present
|
||||
- [ ] `local` feature gate declared but `alknet-tty-local` dependency not yet wired (added in `tty/local-feature-reexport`)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/README.md — crate index
|
||||
- docs/architecture/crates/tty/overview.md — crate overview, dependencies, feature gates, backend location map
|
||||
- docs/architecture/decisions/003-crate-decomposition.md — ADR-003 (Amendment 2: alknet-tty depends on alknet-core only)
|
||||
- docs/architecture/decisions/052-alknet-tty-wire-format-and-two-carriage.md — ADR-052
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md — ADR-054 (sibling crate behind `local` feature)
|
||||
- docs/architecture/decisions/057-alknet-tty-no-alknet-call-dep.md — ADR-057 (no alknet-call dependency)
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the foundational setup task for alknet-tty. All subsequent tty tasks
|
||||
> depend on this one. The crate is intentionally lean — no backend deps. The
|
||||
> `local` feature gate is declared here but the `alknet-tty-local` dependency
|
||||
> is wired in a later task (`tty/local-feature-reexport`) once the sibling crate
|
||||
> exists. The POC at `/workspace/alknet-tty-poc/` is the reference implementation
|
||||
> for the wire codec, control messages, and the local PTY bridge.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,154 @@
|
||||
---
|
||||
id: tty/integration-test
|
||||
name: "End-to-end integration test: LocalTtyBackend + drive_session over real commands"
|
||||
status: pending
|
||||
depends_on: [tty/local-feature-reexport]
|
||||
scope: broad
|
||||
risk: medium
|
||||
impact: phase
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Write end-to-end integration tests that exercise the full stack:
|
||||
`LocalTtyBackend` (PTY and pipe modes) + `TtyAdapter::drive_session` over a
|
||||
`tokio::io::duplex` (transport stand-in for a QUIC bidi stream), running real
|
||||
commands. These tests validate the two crates work together through the
|
||||
`TtyBackend` trait seam and that the wire-format invariants (exit-chunk-is-last,
|
||||
ADR-055; kill-on-Drop, ADR-056) hold with a real backend, not just the
|
||||
`MockBackend` from `tty/adapter`.
|
||||
|
||||
These tests live in `alknet-tty`'s test suite (with `--features local` so the
|
||||
local backend is available) OR in `alknet-tty-local`'s test suite (depending
|
||||
on `alknet-tty` for the adapter). The natural home is `alknet-tty`'s
|
||||
`tests/` directory with a `local` feature gate, since the adapter is in
|
||||
`alknet-tty` and the test exercises the adapter + backend together. Use
|
||||
`#[cfg(feature = "local")]` on the test module.
|
||||
|
||||
### Test scenarios
|
||||
|
||||
All scenarios use `tokio::io::duplex` as the bidi stream stand-in (the POC's
|
||||
pattern). The test acts as the client: writes the negotiation frame, sends
|
||||
stdin/control chunks, reads stdout/stderr/control chunks, asserts the
|
||||
exit chunk and stream close.
|
||||
|
||||
#### PTY mode (`terminal: Some`)
|
||||
|
||||
1. **Happy path (echo)**: negotiate `{backend:"local", tty:{cols:80,rows:24}, cmd:["echo","hello"]}`,
|
||||
read stdout chunks until the exit chunk, assert stdout contains "hello",
|
||||
exit code is 0, exit chunk is the last chunk before stream close.
|
||||
2. **Interactive (cat)**: negotiate `cmd:["cat"]`, write stdin chunks, read
|
||||
them back via stdout, send `eof` control chunk, await exit 0. Assert
|
||||
stdin round-trips through the PTY.
|
||||
3. **Resize**: negotiate `cmd:["cat"]`, send a `resize` control chunk, assert
|
||||
no error (the PTY resizes). Send `eof`, await exit.
|
||||
4. **Signal (SIGINT, Unix)**: negotiate `cmd:["sleep","60"]`, send a `signal`
|
||||
control chunk with `name:"INT"`, await the exit chunk. Assert exit code is
|
||||
signal-terminated (negative, e.g., -2 for SIGINT, or 130). Assert the
|
||||
child is reaped (no zombie).
|
||||
5. **Process-group signal (Unix)**: negotiate `cmd:["bash","-c","sleep 60"]`,
|
||||
send `signal:"INT"`, assert the `sleep` child also receives the signal
|
||||
(the process group is targeted). This validates REQ-TTY-02 end-to-end.
|
||||
6. **Stdin EOF (zero-length chunk)**: negotiate `cmd:["cat"]`, send a
|
||||
zero-length stdin chunk (the sentinel), assert the backend's stdin closes,
|
||||
stdout drains, exit chunk is sent.
|
||||
7. **Cancel cleanup (ADR-056)**: negotiate `cmd:["sleep","60"]`, drop the
|
||||
duplex (simulating connection drop) mid-session, assert the child is
|
||||
killed (no orphan). Use a short delay then check the process is gone
|
||||
(e.g., via `kill(pid, 0)` returning ESRCH, or a `ps` check, or a
|
||||
`tokio::process` tracker).
|
||||
8. **Exit-chunk-is-last**: in the happy path, assert no stdout chunk arrives
|
||||
after the exit chunk. Read all chunks, find the exit chunk, assert it is
|
||||
the last chunk before stream close.
|
||||
|
||||
#### Pipe mode (`terminal: None`)
|
||||
|
||||
9. **Happy path (echo)**: negotiate `{backend:"local", tty:null, cmd:["echo","hello"]}`,
|
||||
read stdout chunks, assert "hello", exit 0. Assert stderr is empty.
|
||||
10. **Separate stderr**: negotiate `cmd:["sh","-c","echo out; echo err >&2"]`,
|
||||
assert stdout stream receives "out", stderr stream receives "err" (as
|
||||
stderr chunks, stream_type 2), exit 0.
|
||||
11. **Signal (SIGTERM, Unix)**: negotiate `cmd:["sleep","60"]`, send
|
||||
`signal:"TERM"`, await exit, assert signal-terminated.
|
||||
12. **Cancel cleanup (ADR-056)**: negotiate `cmd:["sleep","60"]`, drop the
|
||||
duplex mid-session, assert the child is killed (no orphan).
|
||||
13. **Resize no-op**: negotiate `cmd:["cat"]`, send a `resize` control chunk,
|
||||
assert no error (PipeControl::resize is a no-op). Send `eof`, await exit.
|
||||
|
||||
#### Negotiation errors
|
||||
|
||||
14. **unknown_backend**: negotiate `{backend:"kubernetes",...}`, assert the
|
||||
error response `{"error":"unknown_backend","backend":"kubernetes"}`,
|
||||
stream closes, no raw mode (first byte of the error frame is `0x00`).
|
||||
15. **malformed_negotiation (bad JSON)**: write garbage bytes as the
|
||||
negotiation frame, assert `{"error":"malformed_negotiation",...}`.
|
||||
16. **malformed_negotiation (carriage != raw)**: negotiate
|
||||
`{carriage:"json",...}`, assert `malformed_negotiation`.
|
||||
17. **malformed_negotiation (empty cmd)**: negotiate `{cmd:[]}`, assert
|
||||
`malformed_negotiation`.
|
||||
18. **allocate_failed**: (harder to trigger with the local backend — skip or
|
||||
use a non-existent binary) negotiate `cmd:["/nonexistent"]`, assert
|
||||
`allocate_failed` (the spawn fails).
|
||||
|
||||
### Test harness
|
||||
|
||||
Build a small test helper that wraps the client-side wire protocol:
|
||||
- `write_negotiation(frame)` — serialize + length-prefix + write
|
||||
- `write_chunk(stream_type, bytes)` — write a raw chunk
|
||||
- `write_control(ControlMessage)` — serialize + write as stream_type 3
|
||||
- `read_chunk()` — read a chunk, return `(stream_type, bytes)`
|
||||
- `read_error_frame()` — read the length-prefixed JSON error (first byte `0x00`)
|
||||
- `read_until_exit()` — read chunks until the exit control chunk, return
|
||||
(stdout_bytes, stderr_bytes, exit_code)
|
||||
|
||||
This helper can live in a `tests/common/` module or be reused from the
|
||||
adapter's unit tests (the `MockBackend` tests in `tty/adapter` use a similar
|
||||
pattern).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Integration test module in `alknet-tty/tests/` (or `alknet-tty-local/tests/`), `#[cfg(feature = "local")]` gated
|
||||
- [ ] PTY happy path (echo): stdout contains "hello", exit 0, exit chunk last
|
||||
- [ ] PTY interactive (cat): stdin round-trips, eof → exit 0
|
||||
- [ ] PTY resize: control chunk accepted, no error
|
||||
- [ ] PTY signal (SIGINT): exit code signal-terminated, child reaped (Unix)
|
||||
- [ ] PTY process-group signal: shell's child receives the signal (REQ-TTY-02, Unix)
|
||||
- [ ] PTY stdin EOF (zero-length chunk): backend stdin closes, exit chunk sent
|
||||
- [ ] PTY cancel cleanup: drop duplex → child killed, no orphan (ADR-056)
|
||||
- [ ] PTY exit-chunk-is-last: no stdout chunk after exit chunk
|
||||
- [ ] Pipe happy path (echo): stdout "hello", stderr empty, exit 0
|
||||
- [ ] Pipe separate stderr: stdout "out", stderr "err", exit 0
|
||||
- [ ] Pipe signal (SIGTERM): exit signal-terminated (Unix)
|
||||
- [ ] Pipe cancel cleanup: drop duplex → child killed, no orphan (ADR-056)
|
||||
- [ ] Pipe resize no-op: control chunk accepted, no error
|
||||
- [ ] unknown_backend error: error response, stream closes, first byte `0x00`
|
||||
- [ ] malformed_negotiation (bad JSON, bad carriage, empty cmd): error response
|
||||
- [ ] allocate_failed (nonexistent binary): error response
|
||||
- [ ] Test helper for client-side wire protocol (negotiation, chunks, control, error frames)
|
||||
- [ ] `cargo test -p alknet-tty --features local` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty --features local` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-adapter.md — session lifecycle, negotiation errors
|
||||
- docs/architecture/crates/tty/tty-local.md — PTY and pipe mode behavior
|
||||
- docs/architecture/decisions/055-exit-code-on-control-chunk.md — ADR-055 (exit-chunk-is-last)
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md — ADR-056 (cancel cleanup)
|
||||
- /workspace/alknet-tty-poc/tests/integration.rs — the POC's integration tests (reference)
|
||||
- /workspace/alknet-tty-poc/tests/signal.rs — the POC's SIGINT-forwarding test (REQ-TTY-02)
|
||||
|
||||
## Notes
|
||||
|
||||
> These are the tests that validate the two crates work together through the
|
||||
> `TtyBackend` trait seam. The `MockBackend` tests in `tty/adapter` validate
|
||||
> the adapter in isolation; these tests validate the adapter + a real backend.
|
||||
> The cancel-cleanup tests (ADR-056) are the most important — they confirm no
|
||||
> orphaned processes when the connection drops mid-session. The
|
||||
> process-group signal test (REQ-TTY-02) confirms Ctrl-C reaches a shell's
|
||||
> children, not just the shell. Unix-only tests (signal, process-group) use
|
||||
> `#[cfg(unix)]`.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,106 @@
|
||||
---
|
||||
id: tty/local-feature-reexport
|
||||
name: Wire alknet-tty `local` feature gate and re-export LocalTtyBackend
|
||||
status: pending
|
||||
depends_on: [tty/review-tty, tty-local/review-tty-local]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Wire the `local` feature gate on `alknet-tty` (declared in `tty/crate-init`)
|
||||
to re-export `LocalTtyBackend` from `alknet-tty-local`. This is the seam
|
||||
between the two crates (ADR-054): a consumer that wants the local backend
|
||||
enables `features = ["local"]` and gets `alknet_tty::local::LocalTtyBackend`;
|
||||
a consumer that only wants docker/ssh uses the default features and depends
|
||||
on the backend crate directly — no `portable_pty` in the dependency tree.
|
||||
|
||||
### Cargo.toml (alknet-tty)
|
||||
|
||||
The `local` feature was declared in `tty/crate-init` but the
|
||||
`alknet-tty-local` dependency was not yet wired. Add it as an optional
|
||||
dependency activated by the `local` feature:
|
||||
|
||||
```toml
|
||||
[features]
|
||||
default = []
|
||||
local = ["dep:alknet-tty-local"]
|
||||
|
||||
[dependencies]
|
||||
alknet-tty-local = { path = "../alknet-tty-local", optional = true }
|
||||
```
|
||||
|
||||
### Re-export module (alknet-tty/src/lib.rs)
|
||||
|
||||
Add a `local` module gated on the feature, re-exporting `LocalTtyBackend`:
|
||||
|
||||
```rust
|
||||
#[cfg(feature = "local")]
|
||||
pub mod local {
|
||||
pub use alknet_tty_local::LocalTtyBackend;
|
||||
}
|
||||
```
|
||||
|
||||
A consumer with `features = ["local"]` accesses it as
|
||||
`alknet_tty::local::LocalTtyBackend`.
|
||||
|
||||
### Verification
|
||||
|
||||
- `cargo check -p alknet-tty` (default features) — succeeds, no
|
||||
`portable_pty` in the tree.
|
||||
- `cargo check -p alknet-tty --features local` — succeeds,
|
||||
`alknet_tty::local::LocalTtyBackend` is accessible.
|
||||
- `cargo tree -p alknet-tty --features local` shows `portable_pty` pulled in
|
||||
via `alknet-tty-local`; `cargo tree -p alknet-tty` (default) does NOT show
|
||||
`portable_pty`.
|
||||
- `cargo clippy -p alknet-tty --features local` succeeds with no warnings.
|
||||
|
||||
### Assembly-layer example
|
||||
|
||||
Document (in a doc comment or the crate root) the assembly pattern:
|
||||
|
||||
```rust
|
||||
let mut backends = HashMap::new();
|
||||
backends.insert("local".into(),
|
||||
Arc::new(alknet_tty::local::LocalTtyBackend::new()) as Arc<dyn alknet_tty::TtyBackend>);
|
||||
let tty_adapter = alknet_tty::adapter::TtyAdapter::new(backends);
|
||||
```
|
||||
|
||||
This is the pattern the assembly layer (the CLI binary) uses to construct and
|
||||
register the local backend. A docker-only deployment registers `docker`
|
||||
instead; a mixed deployment registers both.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `alknet-tty` Cargo.toml has `alknet-tty-local` as an optional dependency, `path = "../alknet-tty-local"`
|
||||
- [ ] `local` feature activates `dep:alknet-tty-local`
|
||||
- [ ] `alknet-tty/src/lib.rs` has `#[cfg(feature = "local")] pub mod local` re-exporting `LocalTtyBackend`
|
||||
- [ ] `cargo check -p alknet-tty` (default features) succeeds
|
||||
- [ ] `cargo check -p alknet-tty --features local` succeeds
|
||||
- [ ] `cargo tree -p alknet-tty` (default) does NOT include `portable_pty`
|
||||
- [ ] `cargo tree -p alknet-tty --features local` includes `portable_pty` via `alknet-tty-local`
|
||||
- [ ] `cargo clippy -p alknet-tty --features local` succeeds with no warnings
|
||||
- [ ] `cargo test -p alknet-tty --features local` succeeds
|
||||
- [ ] Assembly-layer pattern documented (doc comment or crate root)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/overview.md — §"Feature Gates", §"Backend Location Map"
|
||||
- docs/architecture/crates/tty/tty-backend.md — §"Backend registration and the assembly layer"
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md — ADR-054 (sibling crate behind `local` feature)
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the seam between the two crates. The feature gate ensures a
|
||||
> docker-only or ssh-only deployment doesn't pull in `portable_pty`. The
|
||||
> `cargo tree` check is the verification: `portable_pty` appears only with
|
||||
> `--features local`. This task depends on both review tasks
|
||||
> (`tty/review-tty`, `tty-local/review-tty-local`) because it wires the two
|
||||
> reviewed crates together.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,203 @@
|
||||
---
|
||||
id: tty/negotiation
|
||||
name: Implement negotiation frame (NegotiateRequest, length-prefixed framing, error response)
|
||||
status: pending
|
||||
depends_on: [tty/wire-codec]
|
||||
scope: narrow
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the Phase 1 "JSON carriage" negotiation frame in
|
||||
`src/negotiation.rs`. The client opens a bidi stream and writes a single
|
||||
length-prefixed JSON frame carrying the terminal parameters, backend selector,
|
||||
command, and environment. After this frame, the stream switches to raw chunks
|
||||
(task `tty/wire-codec`). The framing is self-contained in alknet-tty (ADR-057)
|
||||
— a 4-byte big-endian length prefix + UTF-8 JSON body.
|
||||
|
||||
### NegotiateRequest struct
|
||||
|
||||
```rust
|
||||
#[derive(Deserialize)]
|
||||
pub struct NegotiateRequest {
|
||||
pub carriage: String, // "raw" in v1; any other value → malformed_negotiation
|
||||
pub backend: String, // backend selector key ("local", "docker", "ssh")
|
||||
pub tty: Option<TerminalParamsWire>, // None = pipe mode (ADR-054)
|
||||
pub cmd: Vec<String>, // argv[0] + args; non-empty
|
||||
#[serde(default)]
|
||||
pub cwd: Option<PathBuf>, // None = inherit/default
|
||||
#[serde(default)]
|
||||
pub env: HashMap<String, String>, // empty = inherit
|
||||
#[serde(default)]
|
||||
pub backend_params: serde_json::Map<String, serde_json::Value>, // opaque; backend-deserialized
|
||||
// plus backend-specific fields, captured into backend_params via serde(flatten)
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
pub struct TerminalParamsWire {
|
||||
pub term: Option<String>, // None = backend default
|
||||
pub cols: u16,
|
||||
pub rows: u16,
|
||||
#[serde(default)]
|
||||
pub pixel_width: u16,
|
||||
#[serde(default)]
|
||||
pub pixel_height: u16,
|
||||
#[serde(default)]
|
||||
pub modes: serde_json::Value, // reserved — OQ-44; backends MUST ignore content in v1
|
||||
}
|
||||
```
|
||||
|
||||
The `serde(flatten)` for backend-specific fields means the negotiation frame's
|
||||
top-level JSON object carries both the shared fields (`carriage`, `backend`,
|
||||
`tty`, `cmd`, `cwd`, `env`) and the backend-specific fields (e.g.,
|
||||
`"container": "abc123"` for docker); the latter land in `backend_params`.
|
||||
|
||||
### Validation
|
||||
|
||||
- `carriage` MUST be `"raw"` (else `malformed_negotiation`).
|
||||
- `cmd` MUST be non-empty (else `malformed_negotiation`).
|
||||
- `backend` MUST be a registered backend key (else `unknown_backend`) — this
|
||||
check happens in the adapter (task `tty/adapter`), not here; this module
|
||||
only parses.
|
||||
- Backend-specific params validation is the backend's job (in `allocate()`).
|
||||
|
||||
### Length-prefixed framing
|
||||
|
||||
A self-contained ~30-line reader/writer on tokio's `AsyncRead`/`AsyncWrite`:
|
||||
|
||||
```rust
|
||||
pub struct NegotiationReader<R: AsyncRead + Unpin> { /* ... */ }
|
||||
impl<R: AsyncRead + Unpin> NegotiationReader<R> {
|
||||
pub fn new(reader: R) -> Self;
|
||||
pub fn into_inner(self) -> R;
|
||||
/// Read a 4-byte BE length prefix, bounds-check, read N bytes.
|
||||
pub async fn read_frame(&mut self) -> Result<Bytes, NegotiationError>;
|
||||
}
|
||||
|
||||
pub struct NegotiationWriter<W: AsyncWrite + Unpin> { /* ... */ }
|
||||
impl<W: AsyncWrite + Unpin> NegotiationWriter<W> {
|
||||
pub fn new(writer: W) -> Self;
|
||||
pub fn into_inner(self) -> W;
|
||||
/// Write a 4-byte BE length prefix + the JSON body.
|
||||
pub async fn write_frame(&mut self, body: &[u8]) -> Result<(), NegotiationError>;
|
||||
}
|
||||
```
|
||||
|
||||
The reader bounds-checks the length against `MAX_CHUNK_LEN` (from `wire.rs`)
|
||||
so a malformed length prefix can't trigger an oversized allocation. The POC
|
||||
used a 1 MiB cap; the crate uses `MAX_CHUNK_LEN` (16 MiB) to match the raw
|
||||
chunk limit and to make the framing-disambiguation trick sound (see below).
|
||||
|
||||
### Error response shape
|
||||
|
||||
If the server cannot allocate the session, it sends a JSON error response in
|
||||
the same length-prefixed framing and closes the stream without entering raw
|
||||
mode:
|
||||
|
||||
```json
|
||||
{ "error": "unknown_backend", "backend": "kubernetes" }
|
||||
```
|
||||
|
||||
| Error | When | Shape |
|
||||
|-------|------|------|
|
||||
| `unknown_backend` | the `backend` string is not in the adapter's backend map | `{"error":"unknown_backend","backend":"..."}` |
|
||||
| `malformed_negotiation` | the negotiation frame failed to parse or failed validation | `{"error":"malformed_negotiation","message":"..."}` |
|
||||
| `allocate_failed` | `backend.allocate()` returned a `TtyError` | `{"error":"allocate_failed","message":"..."}` |
|
||||
|
||||
Provide a helper to serialize an error response:
|
||||
|
||||
```rust
|
||||
pub fn error_response_bytes(error: &str, fields: &[(&str, &str)]) -> serde_json::Result<Vec<u8>>;
|
||||
```
|
||||
|
||||
### Framing disambiguation (success vs error)
|
||||
|
||||
Both a successful allocation (raw chunks) and a failed allocation (JSON error
|
||||
frame) begin with bytes the client must read before knowing which framing
|
||||
applies. The disambiguation is by the first byte:
|
||||
|
||||
- A JSON error frame's 4-byte big-endian length prefix always starts with
|
||||
`0x00` (error frames MUST be under 16 MiB — `MAX_CHUNK_LEN` — so the high
|
||||
byte is zero; this is a wire-format invariant, not an assumption).
|
||||
- A raw chunk's first byte is a `stream_type` in `{0, 1, 2, 3}`. A stream_type
|
||||
of `0` (stdin from server) is invalid — the server never sends stdin chunks.
|
||||
|
||||
So the client distinguishes: read the first byte; if it is `0x00`, interpret
|
||||
the next 4 bytes as a big-endian length prefix and read that many bytes as a
|
||||
JSON error frame; otherwise interpret it as a `stream_type` byte and continue
|
||||
reading the raw chunk header. This is a one-way-door wire-format invariant
|
||||
(ADR-052). The `NegotiationWriter::write_frame` for an error MUST ensure the
|
||||
body is under 16 MiB (the high byte of the length is `0x00`).
|
||||
|
||||
### NegotiationError
|
||||
|
||||
```rust
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum NegotiationError {
|
||||
#[error("io: {0}")]
|
||||
Io(#[from] std::io::Error),
|
||||
#[error("connection closed")]
|
||||
ConnectionClosed,
|
||||
#[error("frame too large: {0}")]
|
||||
FrameTooLarge(u32),
|
||||
#[error("json: {0}")]
|
||||
Json(#[from] serde_json::Error),
|
||||
}
|
||||
```
|
||||
|
||||
### Tests
|
||||
|
||||
- Round-trip: write a `NegotiateRequest` as JSON, read the frame, parse, assert
|
||||
fields match.
|
||||
- `serde(flatten)` test: a frame with `{"carriage":"raw","backend":"local",...,"container":"abc"}`
|
||||
parses `container` into `backend_params`.
|
||||
- Validation: `carriage != "raw"` → adapter rejects (this module parses; the
|
||||
adapter checks the value). Test that the struct deserializes regardless and
|
||||
the adapter-side check is a string comparison.
|
||||
- `FrameTooLarge` on length > MAX_CHUNK_LEN.
|
||||
- `ConnectionClosed` on truncated frame.
|
||||
- Error response serialization produces the expected JSON shape.
|
||||
- Framing disambiguation: an error frame's first byte is `0x00` (write a
|
||||
frame, read the first byte, assert `0x00`).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `NegotiateRequest` struct with all fields, `serde(flatten)` for `backend_params`
|
||||
- [ ] `TerminalParamsWire` struct with `term`, `cols`, `rows`, `pixel_width`, `pixel_height`, `modes`
|
||||
- [ ] `NegotiationReader::read_frame` reads 4-byte BE length, bounds-checks against `MAX_CHUNK_LEN`, reads body
|
||||
- [ ] `NegotiationWriter::write_frame` writes 4-byte BE length + body
|
||||
- [ ] `NegotiationError` with `Io`, `ConnectionClosed`, `FrameTooLarge`, `Json`
|
||||
- [ ] `error_response_bytes` helper produces `{"error":"...","field":"..."}`
|
||||
- [ ] Error frames are under 16 MiB (high byte of length prefix is `0x00`)
|
||||
- [ ] `into_inner` on reader/writer reclaims the underlying stream for raw-chunk use
|
||||
- [ ] Round-trip unit test for `NegotiateRequest` (all fields)
|
||||
- [ ] Unit test: `serde(flatten)` captures backend-specific fields into `backend_params`
|
||||
- [ ] Unit test: `FrameTooLarge` on length > MAX_CHUNK_LEN
|
||||
- [ ] Unit test: `ConnectionClosed` on truncated frame
|
||||
- [ ] Unit test: error response first byte is `0x00` (framing disambiguation)
|
||||
- [ ] `cargo test -p alknet-tty` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-wire.md — §"Phase 1: Negotiation Frame", §"Constraints" (negotiation errors)
|
||||
- docs/architecture/crates/tty/tty-adapter.md — §"Negotiation Errors" (framing disambiguation)
|
||||
- docs/architecture/decisions/052-alknet-tty-wire-format-and-two-carriage.md — ADR-052
|
||||
- docs/architecture/decisions/057-alknet-tty-no-alknet-call-dep.md — ADR-057 (self-contained framing)
|
||||
- /workspace/alknet-tty-poc/src/session.rs — `NegotiationReader` (the ~30-line reference)
|
||||
|
||||
## Notes
|
||||
|
||||
> The framing is self-contained (ADR-057) — do NOT depend on alknet-call's
|
||||
> `FrameFramedReader`. The `MAX_CHUNK_LEN` constant from `wire.rs` is the
|
||||
> bounds-check ceiling for both the negotiation frame and the raw chunks,
|
||||
> which is what makes the `0x00`-as-length-prefix vs `0x00`-as-invalid-stream_type
|
||||
> disambiguation sound. The POC's `NegotiationReader` used a 1 MiB cap; the
|
||||
> crate uses `MAX_CHUNK_LEN` (16 MiB) per the spec.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,145 @@
|
||||
---
|
||||
id: tty/review-tty-final
|
||||
name: Final review of alknet-tty + alknet-tty-local for merge readiness
|
||||
status: pending
|
||||
depends_on: [tty/integration-test]
|
||||
scope: broad
|
||||
risk: low
|
||||
impact: project
|
||||
level: review
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Final review of the `alknet-tty` and `alknet-tty-local` crates for merge
|
||||
readiness. This is the last quality checkpoint before the tty work is
|
||||
considered complete. It validates the two crates as a unit: the core crate's
|
||||
wire format + adapter + trait, the local backend's PTY/pipe implementations,
|
||||
the feature-gate seam, and the end-to-end integration tests.
|
||||
|
||||
### Review Checklist
|
||||
|
||||
1. **Cross-crate seam**:
|
||||
- `alknet-tty` `local` feature gate activates `dep:alknet-tty-local`
|
||||
- `alknet_tty::local::LocalTtyBackend` re-export works with `--features local`
|
||||
- `cargo tree -p alknet-tty` (default) does NOT include `portable_pty`
|
||||
- `cargo tree -p alknet-tty --features local` includes `portable_pty` via `alknet-tty-local`
|
||||
- `LocalTtyBackend` implements `alknet_tty::TtyBackend` (the trait compiles against the impl)
|
||||
- Assembly-layer pattern documented
|
||||
|
||||
2. **Wire format invariants (ADR-052)**:
|
||||
- Two-carriage model: JSON negotiation frame, then raw chunks
|
||||
- Fixed channel set (0-3), no extension escape hatch
|
||||
- `MAX_CHUNK_LEN` = 16 MiB shared between negotiation and raw chunks
|
||||
- Framing disambiguation: error frame first byte `0x00`, raw chunk first byte 1-3
|
||||
- Self-contained framing (no alknet-call dependency, ADR-057)
|
||||
|
||||
3. **Exit-chunk ordering (ADR-055)**:
|
||||
- Exit chunk is the last chunk before stream close
|
||||
- Adapter waits for stdout/stderr pumps AND exit_code resolve before exit chunk
|
||||
- Exit error → `{"type":"exit","code":-1}`
|
||||
- Integration test confirms exit-chunk-is-last with a real backend
|
||||
|
||||
4. **Cancel-cleanup (ADR-056)**:
|
||||
- `LocalExitFuture` (PTY) has kill-on-Drop guard (`ChildKiller::kill(SIGHUP)`)
|
||||
- `PipeExitFuture` (pipe) has kill-on-Drop guard (`Child::start_kill()`)
|
||||
- Both disarm on resolve (no-op Drop on happy path)
|
||||
- Neither is a bare `oneshot::Receiver` / bare `Child::wait()`
|
||||
- Integration tests confirm no orphaned processes on connection drop (PTY + pipe)
|
||||
|
||||
5. **REQ-TTY-01 (backends need not be natively async)**:
|
||||
- PTY mode uses three std threads (reader, writer, waiter) + tokio mpsc/oneshot
|
||||
- Trait's adapter-facing types (`AsyncWrite`, `Stream`, `BoxFuture`, `TtyControl`) are the contract
|
||||
- Documented as a supported implementation strategy
|
||||
|
||||
6. **REQ-TTY-02 (signal forwarding to process group)**:
|
||||
- PTY mode: `kill(-pgid, sig)` with `kill(pid, sig)` fallback
|
||||
- `portable_pty` child is a session leader (`controlling_tty = true`)
|
||||
- Pipe mode: `kill(pid, sig)` only (documented limitation, no process group)
|
||||
- Unknown names fall back to default kill (SIGHUP for PTY, SIGKILL for pipe)
|
||||
- Integration test confirms process-group targeting (shell's child receives signal)
|
||||
|
||||
7. **Access control (ADR-050)**:
|
||||
- Scope-gate at negotiation (`tty:open` or deployment-configured)
|
||||
- Ownership check via `backend.resource_id()` + `OwnershipProvider::owns()` when wired
|
||||
- `LocalTtyBackend::resource_id()` returns `None` (creates its own resource)
|
||||
|
||||
8. **Dependency hygiene**:
|
||||
- `alknet-tty` depends on `alknet-core` only (no `alknet-call`, no backend deps)
|
||||
- `alknet-tty-local` depends on `alknet-tty` + `portable_pty` + `libc`
|
||||
- No `alknet-core` direct dep in `alknet-tty-local`
|
||||
- No `bollard`/`russh` (future backend crates)
|
||||
|
||||
9. **Pattern consistency**:
|
||||
- `thiserror` for error enums (`TtyError`, `RawError`, `NegotiationError`)
|
||||
- `async-trait` for `TtyBackend` and `ProtocolHandler`
|
||||
- `tracing` for structured logging
|
||||
- `TtyControlHandle::new(Arc::new(...))` wrapping (OQ-43)
|
||||
- `#[cfg(unix)]` gates on `libc::kill` paths
|
||||
- `#[non_exhaustive]` on `TtyError`
|
||||
|
||||
10. **Test coverage**:
|
||||
- Wire codec unit tests (round-trip, error cases)
|
||||
- Control message unit tests (round-trip, signal_from_name)
|
||||
- Negotiation unit tests (round-trip, serde(flatten), error framing, disambiguation)
|
||||
- Backend trait unit tests (MockBackend, TtyControlHandle delegation)
|
||||
- Adapter unit tests (MockBackend over duplex, all scenarios)
|
||||
- PTY mode integration tests (happy, interactive, resize, signal, process-group, cancel, exit-last)
|
||||
- Pipe mode integration tests (happy, stderr, signal, cancel, resize)
|
||||
- Negotiation error integration tests (unknown_backend, malformed, allocate_failed)
|
||||
- All tests pass with `--features local`
|
||||
|
||||
11. **Documentation**:
|
||||
- Crate-level doc comments on both crates
|
||||
- Module-level doc comments
|
||||
- ADR-056 kill-on-Drop contract documented on `TtyHandle.exit_code`
|
||||
- REQ-TTY-01 documented on `TtyBackend` trait
|
||||
- REQ-TTY-02 documented on `PtyControl::signal`
|
||||
- Pipe-mode signal limitation documented on `PipeControl::signal`
|
||||
- Assembly-layer pattern documented
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Cross-crate seam: `local` feature re-export works; `portable_pty` gated correctly
|
||||
- [ ] Wire format invariants (ADR-052) hold: two-carriage, fixed channels, disambiguation
|
||||
- [ ] Exit-chunk-is-last (ADR-055) enforced in adapter, validated in integration tests
|
||||
- [ ] Cancel-cleanup (ADR-056) guards present in both PTY and pipe exit futures; integration tests confirm no orphans
|
||||
- [ ] REQ-TTY-01 satisfied: PTY mode uses three-thread bridge; trait contract documented
|
||||
- [ ] REQ-TTY-02 satisfied: PTY process-group signal forwarding; integration test passes
|
||||
- [ ] Access control (ADR-050): scope-gate + ownership check wired
|
||||
- [ ] Dependency hygiene: alknet-tty lean (core only); alknet-tty-local has portable_pty/libc
|
||||
- [ ] Pattern consistency: thiserror, async-trait, tracing, TtyControlHandle wrapping, cfg(unix)
|
||||
- [ ] All unit + integration tests pass with `--features local`
|
||||
- [ ] `cargo fmt --check` passes for both crates
|
||||
- [ ] `cargo clippy` passes with no warnings for both crates (default + `--features local`)
|
||||
- [ ] Documentation complete (crate, module, contract doc comments)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/README.md
|
||||
- docs/architecture/crates/tty/overview.md
|
||||
- docs/architecture/crates/tty/tty-wire.md
|
||||
- docs/architecture/crates/tty/tty-backend.md
|
||||
- docs/architecture/crates/tty/tty-adapter.md
|
||||
- docs/architecture/crates/tty/tty-local.md
|
||||
- docs/architecture/decisions/052-alknet-tty-wire-format-and-two-carriage.md
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md
|
||||
- docs/architecture/decisions/054-local-tty-backend-sibling-crate.md
|
||||
- docs/architecture/decisions/055-exit-code-on-control-chunk.md
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md
|
||||
- docs/architecture/decisions/057-alknet-tty-no-alknet-call-dep.md
|
||||
|
||||
## Notes
|
||||
|
||||
> This is the final review before the tty work is considered complete. It
|
||||
> validates the two crates as a unit — the core crate's invariants
|
||||
> (exit-chunk-is-last, kill-on-Drop, framing disambiguation) must hold with
|
||||
> the real local backend, not just the MockBackend. The cancel-cleanup
|
||||
> integration tests are the most critical: an orphaned process on connection
|
||||
> drop is the bug ADR-056 exists to prevent. The process-group signal test
|
||||
> (REQ-TTY-02) is the other critical validation — Ctrl-C must reach a
|
||||
> shell's children. If any invariant fails, document and fix before merge.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,135 @@
|
||||
---
|
||||
id: tty/review-tty
|
||||
name: Review alknet-tty core crate for spec conformance before local backend begins
|
||||
status: pending
|
||||
depends_on: [tty/adapter]
|
||||
scope: moderate
|
||||
risk: low
|
||||
impact: phase
|
||||
level: review
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review the alknet-tty core crate implementation for spec conformance, pattern
|
||||
consistency, and correctness before `alknet-tty-local` begins implementation
|
||||
against the `TtyBackend` trait. This is the quality checkpoint at the end of
|
||||
the core crate phase — the trait shape is a one-way door (ADR-053), and the
|
||||
local backend builds against it, so any issues here propagate to the sibling
|
||||
crate.
|
||||
|
||||
### Review Checklist
|
||||
|
||||
1. **Wire codec conformance** (tty-wire.md §"Phase 2"):
|
||||
- `ChunkReader`/`ChunkWriter` with 5-byte header (1 stream_type + 4 BE length)
|
||||
- `STREAM_STDIN`=0, `STREAM_STDOUT`=1, `STREAM_STDERR`=2, `STREAM_CONTROL`=3
|
||||
- `MAX_CHUNK_LEN` = 16 MiB; `ChunkTooLarge` on overflow
|
||||
- `InvalidStreamType` on stream_type > 3
|
||||
- `ConnectionClosed` on clean EOF (not `Io`)
|
||||
- Zero-length chunks are sentinels (codec doesn't special-case; adapter interprets)
|
||||
|
||||
2. **Control messages conformance** (tty-wire.md §"Control Channel"):
|
||||
- `ControlMessage` enum: `Resize`, `Signal`, `Eof`, `Exit`
|
||||
- `#[serde(tag = "type", rename_all = "snake_case")]`
|
||||
- `Exit.code` is `i32` (negative = signal-terminated)
|
||||
- `signal_from_name` for 9 names, `#[cfg(unix)]` gated
|
||||
- Unknown `type` values: `from_slice` returns error; adapter ignores (not a schema catch-all)
|
||||
|
||||
3. **Negotiation conformance** (tty-wire.md §"Phase 1", §"Constraints"):
|
||||
- `NegotiateRequest` with `carriage`, `backend`, `tty`, `cmd`, `cwd`, `env`, `backend_params`
|
||||
- `serde(flatten)` captures backend-specific fields into `backend_params`
|
||||
- `TerminalParamsWire` with `term`, `cols`, `rows`, `pixel_width`, `pixel_height`, `modes`
|
||||
- Length-prefixed framing (4-byte BE + JSON body), self-contained (no alknet-call dep, ADR-057)
|
||||
- `NegotiationReader` bounds-checks against `MAX_CHUNK_LEN`
|
||||
- Error response shape: `{"error":"...","field":"..."}`
|
||||
- Framing disambiguation: error frame first byte is `0x00` (under 16 MiB invariant)
|
||||
|
||||
4. **Backend trait conformance** (tty-backend.md, ADR-053):
|
||||
- `TtyBackend` trait: `allocate()` (async), `resource_id()` (default `None`)
|
||||
- `TtyError` `#[non_exhaustive]`: `AllocFailed`, `WaitFailed`, `Io`, `Backend`
|
||||
- `TtyParams`: `terminal`, `cmd`, `cwd`, `env`, `backend_params` (opaque `serde_json::Map`)
|
||||
- `TerminalParams`: `term`, `cols`, `rows`, `pixel_width`, `pixel_height`, `modes`
|
||||
- `TtyHandle`: `stdin` (Box<dyn AsyncWrite + Send + Unpin>), `stdout` (Pin<Box<dyn Stream<Item=Bytes> + Send>>), `stderr` (Option), `exit_code` (BoxFuture<Result<i32, TtyError>>), `control` (Option<TtyControlHandle>)
|
||||
- `TtyControl` trait: `resize`, `signal` (object-safe, NOT `Clone`)
|
||||
- `TtyControlHandle`: `#[derive(Clone)]`, `Arc<dyn TtyControl + Send + Sync>` newtype (OQ-43)
|
||||
- `From<NegotiateRequest> for TtyParams` (or constructor)
|
||||
- REQ-TTY-01 documented (backends need not be natively async)
|
||||
- ADR-056 kill-on-Drop contract documented on `exit_code` field
|
||||
|
||||
5. **Adapter conformance** (tty-adapter.md):
|
||||
- `TtyAdapter` struct with `backends` (`Arc<HashMap<String, Arc<dyn TtyBackend>>>`), `ownership` (Option)
|
||||
- `impl ProtocolHandler` with `alpn()` = `b"alknet/tty"`
|
||||
- `handle()` loops `accept_bi`, spawns `drive_session` per stream
|
||||
- Negotiation errors: `unknown_backend`, `malformed_negotiation`, `allocate_failed` as JSON in negotiation framing
|
||||
- Three-pump driver: stdout→client, client→backend (stdin + control), exit→exit-chunk
|
||||
- stderr pump concurrent when `stderr` is `Some`
|
||||
- **Exit-chunk-is-last** (ADR-055): stdout/stderr pumps complete AND exit_code resolves before exit chunk
|
||||
- Exit error → `{"type":"exit","code":-1}`
|
||||
- Unknown control `type` ignored; `Exit` from client ignored
|
||||
- Zero-length stdin chunk AND `eof` control both close backend stdin
|
||||
- Scope-gate (`tty:open`) at negotiation
|
||||
- Ownership check via `resource_id()` + `OwnershipProvider::owns()` when wired
|
||||
- Connection drop / stream reset drops `TtyHandle` (ADR-056 cancel-cleanup)
|
||||
- Client write-half close does NOT trigger cancel-cleanup
|
||||
|
||||
6. **Dependency constraints**:
|
||||
- No `portable_pty`, `bollard`, `russh` dependency (lean core)
|
||||
- No `alknet-call` dependency (ADR-057 — self-contained framing)
|
||||
- `alknet-core` is the only alknet dependency
|
||||
- `local` feature gate declared; `alknet-tty-local` not yet wired (later task)
|
||||
|
||||
7. **Pattern consistency**:
|
||||
- `thiserror` for error enums, `Result` propagation
|
||||
- `tracing` for structured logging (debug/warn)
|
||||
- `async-trait` for `TtyBackend` and `ProtocolHandler`
|
||||
- `tokio` runtime idioms (mpsc, oneshot, spawn)
|
||||
|
||||
8. **Test coverage**:
|
||||
- Wire codec round-trip + error cases
|
||||
- Control message round-trip + `signal_from_name`
|
||||
- Negotiation round-trip + `serde(flatten)` + error framing
|
||||
- `MockBackend` + `TtyControlHandle` delegation
|
||||
- Adapter: happy path, exit-chunk-is-last, stdin EOF, control dispatch, unknown control, all 3 negotiation errors, exit error, cancel cleanup, scope gate, ownership check
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] All wire codec types match tty-wire.md §"Phase 2"
|
||||
- [ ] All control message types match tty-wire.md §"Control Channel"
|
||||
- [ ] All negotiation types match tty-wire.md §"Phase 1" + §"Constraints"
|
||||
- [ ] All backend trait types match tty-backend.md (ADR-053)
|
||||
- [ ] Adapter matches tty-adapter.md (session lifecycle, exit ordering, access control)
|
||||
- [ ] Exit-chunk-is-last invariant (ADR-055) enforced in the adapter, not the backend
|
||||
- [ ] ADR-056 kill-on-Drop contract documented on `TtyHandle.exit_code`
|
||||
- [ ] No `portable_pty`/`bollard`/`russh`/`alknet-call` dependency
|
||||
- [ ] `local` feature gate declared, `alknet-tty-local` not yet wired
|
||||
- [ ] `cargo fmt --check -p alknet-tty` passes
|
||||
- [ ] `cargo clippy -p alknet-tty` passes with no warnings
|
||||
- [ ] All tests pass
|
||||
- [ ] `MockBackend` is reusable for the local backend's integration tests
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/README.md
|
||||
- docs/architecture/crates/tty/overview.md
|
||||
- docs/architecture/crates/tty/tty-wire.md
|
||||
- docs/architecture/crates/tty/tty-backend.md
|
||||
- docs/architecture/crates/tty/tty-adapter.md
|
||||
- docs/architecture/decisions/052-alknet-tty-wire-format-and-two-carriage.md
|
||||
- docs/architecture/decisions/053-ttybackend-trait-and-ttyhandle.md
|
||||
- docs/architecture/decisions/055-exit-code-on-control-chunk.md
|
||||
- docs/architecture/decisions/056-backend-cleanup-on-session-cancel.md
|
||||
- docs/architecture/decisions/057-alknet-tty-no-alknet-call-dep.md
|
||||
|
||||
## Notes
|
||||
|
||||
> This review verifies the core crate is spec-conformant before the local
|
||||
> backend builds against the `TtyBackend` trait. The trait shape is one-way
|
||||
> (ADR-053) — any issues here propagate to `alknet-tty-local` and the future
|
||||
> docker/SSH backend crates. Pay special attention to the exit-chunk-is-last
|
||||
> ordering (ADR-055) and the kill-on-Drop contract (ADR-056) — these are the
|
||||
> subtle invariants the POC did not fully enforce. If deviations are found,
|
||||
> document and fix before proceeding to the local backend tasks.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
@@ -0,0 +1,153 @@
|
||||
---
|
||||
id: tty/wire-codec
|
||||
name: Implement raw chunk codec (ChunkReader/ChunkWriter, RawError, stream types)
|
||||
status: pending
|
||||
depends_on: [tty/crate-init]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Implement the raw chunk codec in `src/wire.rs`. This is the Phase 2 "raw
|
||||
carriage" format (ADR-052): after the single JSON negotiation frame, the bidi
|
||||
stream switches to a chunk format for the life of the session.
|
||||
|
||||
The codec is a direct port of the POC's `/workspace/alknet-tty-poc/src/raw.rs`,
|
||||
generalized into the crate's `wire` module. The POC code is the reference; this
|
||||
task ports it verbatim with crate-appropriate doc comments and the `RawError`
|
||||
type exposed publicly.
|
||||
|
||||
### Wire format
|
||||
|
||||
```text
|
||||
[stream_type: u8][length: u32 be][payload bytes]
|
||||
```
|
||||
|
||||
- `stream_type` (1 byte) — the channel:
|
||||
|
||||
| stream_type | channel | direction | payload |
|
||||
|---|---|---|---|
|
||||
| 0 | data-in (stdin) | client→server | raw bytes |
|
||||
| 1 | data-out (stdout) | server→client | raw bytes |
|
||||
| 2 | data-err (stderr) | server→client | raw bytes |
|
||||
| 3 | control | bidirectional | JSON control message |
|
||||
|
||||
`stream_type > 3` is a protocol error (`InvalidStreamType`). There is no
|
||||
extension escape hatch in the byte — a 5th channel is a wire-format change
|
||||
requiring a new ALPN (`alknet/tty/v2` per ADR-006).
|
||||
|
||||
- `length` (4 bytes, big-endian) — payload length in bytes. Max 16 MiB
|
||||
(`MAX_CHUNK_LEN = 16 * 1024 * 1024`). A chunk larger than 16 MiB is a
|
||||
protocol error (`ChunkTooLarge`).
|
||||
|
||||
- `payload` (`length` bytes) — raw bytes (data channels) or UTF-8 JSON (control).
|
||||
|
||||
### Types to implement
|
||||
|
||||
```rust
|
||||
pub const STREAM_STDIN: u8 = 0;
|
||||
pub const STREAM_STDOUT: u8 = 1;
|
||||
pub const STREAM_STDERR: u8 = 2;
|
||||
pub const STREAM_CONTROL: u8 = 3;
|
||||
|
||||
pub const CHUNK_HEADER_LEN: usize = 5; // 1 byte type + 4 bytes length
|
||||
pub const MAX_CHUNK_LEN: u32 = 16 * 1024 * 1024;
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum RawError {
|
||||
#[error("io: {0}")]
|
||||
Io(#[from] std::io::Error),
|
||||
#[error("connection closed")]
|
||||
ConnectionClosed,
|
||||
#[error("invalid chunk header: stream type {0}")]
|
||||
InvalidStreamType(u8),
|
||||
#[error("chunk too large: {0}")]
|
||||
ChunkTooLarge(u32),
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct Chunk {
|
||||
pub stream_type: u8,
|
||||
pub bytes: bytes::Bytes,
|
||||
}
|
||||
|
||||
impl Chunk {
|
||||
pub fn stdin(bytes: bytes::Bytes) -> Self;
|
||||
pub fn stdout(bytes: bytes::Bytes) -> Self;
|
||||
pub fn stderr(bytes: bytes::Bytes) -> Self;
|
||||
pub fn control(bytes: bytes::Bytes) -> Self;
|
||||
}
|
||||
|
||||
pub struct ChunkReader<R: AsyncRead + Unpin> { /* ... */ }
|
||||
impl<R: AsyncRead + Unpin> ChunkReader<R> {
|
||||
pub fn new(reader: R) -> Self;
|
||||
pub fn into_inner(self) -> R;
|
||||
pub async fn read_chunk(&mut self) -> Result<Chunk, RawError>;
|
||||
}
|
||||
|
||||
pub struct ChunkWriter<W: AsyncWrite + Unpin> { /* ... */ }
|
||||
impl<W: AsyncWrite + Unpin> ChunkWriter<W> {
|
||||
pub fn new(writer: W) -> Self;
|
||||
pub fn into_inner(self) -> W;
|
||||
pub async fn write_chunk(&mut self, chunk: &Chunk) -> Result<(), RawError>;
|
||||
pub async fn write_stdin(&mut self, bytes: &[u8]) -> Result<(), RawError>;
|
||||
pub async fn write_control_json(&mut self, json: &[u8]) -> Result<(), RawError>;
|
||||
}
|
||||
```
|
||||
|
||||
### Implementation notes
|
||||
|
||||
- `read_chunk` reads the 5-byte header, validates `stream_type <= 3` (else
|
||||
`InvalidStreamType`), validates `length <= MAX_CHUNK_LEN` (else
|
||||
`ChunkTooLarge`), reads `length` bytes. On `UnexpectedEof` reading the header
|
||||
or payload, return `ConnectionClosed` (not `Io`) — the stream ended cleanly.
|
||||
- `write_chunk` writes the 5-byte header then the payload (if non-empty), then
|
||||
flushes. `write_stdin` and `write_control_json` are convenience helpers.
|
||||
- Zero-length chunks are sentinels (see tty-wire.md §"Sentinels"): a zero-length
|
||||
stdin chunk is EOF from the client; a zero-length stdout chunk is "drained"
|
||||
from the server. The codec does not special-case these — they are just chunks
|
||||
with `length == 0`; the adapter interprets them.
|
||||
|
||||
### Tests
|
||||
|
||||
Port the POC's round-trip behavior: write a chunk, read it back, assert
|
||||
equality. Test all four stream types. Test `InvalidStreamType` (stream_type 4)
|
||||
and `ChunkTooLarge` (length > MAX_CHUNK_LEN). Test `ConnectionClosed` on
|
||||
truncated header/payload. Use `tokio::io::duplex` as the transport stand-in.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `STREAM_STDIN`/`STREAM_STDOUT`/`STREAM_STDERR`/`STREAM_CONTROL` constants defined (0-3)
|
||||
- [ ] `CHUNK_HEADER_LEN` (5) and `MAX_CHUNK_LEN` (16 MiB) constants defined
|
||||
- [ ] `RawError` enum with `Io`, `ConnectionClosed`, `InvalidStreamType`, `ChunkTooLarge`
|
||||
- [ ] `Chunk` struct with `stream_type`/`bytes` fields and `stdin`/`stdout`/`stderr`/`control` constructors
|
||||
- [ ] `ChunkReader::read_chunk` validates stream_type and length, returns `ConnectionClosed` on clean EOF
|
||||
- [ ] `ChunkWriter::write_chunk`/`write_stdin`/`write_control_json` write header + payload + flush
|
||||
- [ ] Round-trip unit tests for all four stream types
|
||||
- [ ] Unit test: `InvalidStreamType` on stream_type > 3
|
||||
- [ ] Unit test: `ChunkTooLarge` on length > MAX_CHUNK_LEN
|
||||
- [ ] Unit test: `ConnectionClosed` on truncated header and truncated payload
|
||||
- [ ] `cargo test -p alknet-tty` succeeds
|
||||
- [ ] `cargo clippy -p alknet-tty` succeeds with no warnings
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/crates/tty/tty-wire.md — wire format spec (§"Phase 2: Raw Chunk Format", §"Sentinels")
|
||||
- docs/architecture/decisions/052-alknet-tty-wire-format-and-two-carriage.md — ADR-052
|
||||
- /workspace/alknet-tty-poc/src/raw.rs — the reference implementation to port
|
||||
|
||||
## Notes
|
||||
|
||||
> This is a near-verbatim port of the POC's `raw.rs`. The codec is
|
||||
> transport-agnostic (works over any `AsyncRead`/`AsyncWrite`); the adapter
|
||||
> task wires it to QUIC bidi streams. The `MAX_CHUNK_LEN` constant is shared
|
||||
> with the negotiation module (the framing-disambiguation trick depends on
|
||||
> error frames being under 16 MiB so the high byte of the length prefix is
|
||||
> `0x00` — see tty-adapter.md §"Negotiation errors").
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
Reference in new issue
Block a user