From a3a2f4c2ae6d2340ef6cc1e1f37924c196457215 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 18:42:31 +0000 Subject: [PATCH 1/8] fix(cli): clamp the attach status line to the terminal width MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `drive` or `passthrough` status line is painted on the bottom row inside an ESC 7 / ESC 8 (DECSC/DECRC) pair. That is only safe while the label fits: a label wider than the pane wraps past the last row, which scrolls the screen. Because the label sits ON the bottom row, the next repaint scrolls again — promoting the previous status line into the scrollback as content and eating a row of the agent's TUI each time. A short burst of pending-count changes turns a single status bar into a growing stack of them with the agent's screen shredded behind it. The `drive` label is 87 columns, so this fired on a standard 80-column terminal, not just narrow split panes. At 66 columns six repaints cost six rows of agent output. `renderStatusLine` now takes `cols` and truncates from the middle, so the verb and agent name stay readable and the `Ctrl+…` hints survive. Both verbs track the local terminal width alongside its height (initial size, snapshot, and resize). Adds tests/e2e/tic-tac-toe: an always-on regression guard that replays the real status-line output through a headless terminal emulator (byte assertions cannot distinguish "painted once" from "painted six times while scrolling the screen away"), plus an opt-in live scenario where three PTY agents play a full game of tic-tac-toe over the relay protocol with a `view` client watching each through a real PTY. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- CHANGELOG.md | 1 + packages/cli/src/cli/lib/attach-drive.test.ts | 52 ++- packages/cli/src/cli/lib/attach-drive.ts | 15 +- .../src/cli/lib/attach-passthrough.test.ts | 12 + .../cli/src/cli/lib/attach-passthrough.ts | 21 +- packages/cli/src/cli/lib/attach.test.ts | 47 +++ packages/cli/src/cli/lib/attach.ts | 49 +++ tests/e2e/tic-tac-toe/README.md | 93 +++++ tests/e2e/tic-tac-toe/harness.ts | 366 ++++++++++++++++++ tests/e2e/tic-tac-toe/pty-run.py | 137 +++++++ tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts | 298 ++++++++++++++ 11 files changed, 1085 insertions(+), 6 deletions(-) create mode 100644 tests/e2e/tic-tac-toe/README.md create mode 100644 tests/e2e/tic-tac-toe/harness.ts create mode 100644 tests/e2e/tic-tac-toe/pty-run.py create mode 100644 tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ac358390b..587201c17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The published CLI now actually reports telemetry. The npm package is plain `tsc` output with no key injection step, and the bun standalone's `--define` targeted a literal `process.env.AGENT_RELAY_POSTHOG_KEY` that the code never read (it used a computed `process.env[name]` lookup), so **both** installable artifacts shipped with telemetry silently disabled — every `cli_command_run`, `workflow_run`, `cloud_auth`, `agent_relay_tool_call`, `setup_init`, `swarm_run`, and `bridge_spawn` event was dropped. Only the Rust broker was reporting. - Opting out of telemetry (`AGENT_RELAY_TELEMETRY_DISABLED` or `DO_NOT_TRACK`) now keeps your cloud identity out of child process environments, including identity an ancestor process or your shell had already exported. The identity env vars — one of which carries your email — previously reached every spawned process, including third-party harness CLIs, even when opted out. - Identity forwarding to the Relaycast gateway is no longer gated on the local process carrying a PostHog key. An npm-installed CLI bakes no key, so it previously forwarded no identity at all and every hosted event fell back to being keyed on the workspace. Forwarding now follows the telemetry preference alone. +- `node agent attach --mode drive|passthrough` truncates its status line to the terminal width, preventing repeated repaints from scrolling away agent output. - `node agent attach --mode view` now exits on the first Ctrl-C instead of waiting for a WebSocket close handshake. - The broker now sends its anonymous telemetry id (`X-Agent-Relay-Distinct-Id`) and origin actor with its Relaycast requests, so hosted usage can be attributed to an install instead of only to a workspace. The id header is omitted when telemetry is opted out; requests and origin actor are unaffected. - The broker now reads its telemetry preference and machine-id files from `AGENT_RELAY_DATA_DIR` when set, matching the CLI. It previously only read `~/.agentworkforce/relay/telemetry.json`, so an opt-out written by `agent-relay telemetry disable` under a configured data directory was ignored. diff --git a/packages/cli/src/cli/lib/attach-drive.test.ts b/packages/cli/src/cli/lib/attach-drive.test.ts index cf3e8fe6f..d95150edb 100644 --- a/packages/cli/src/cli/lib/attach-drive.test.ts +++ b/packages/cli/src/cli/lib/attach-drive.test.ts @@ -566,9 +566,15 @@ describe('KeybindParser', () => { }); }); +/** Strip the save/position/clear/reverse-video wrapper down to the visible label. */ +function stripStatusLineAnsi(rendered: string): string { + // eslint-disable-next-line no-control-regex -- matching the raw ESC bytes this module emits + return rendered.replace(/\x1b(?:[78]|\[[0-9;]*[A-Za-z])/g, ''); +} + describe('renderStatusLine', () => { it('includes agent name, mode, pending count, and detach hint', () => { - const out = renderStatusLine({ name: 'Alice', mode: 'manual_flush', pending: 3 }); + const out = renderStatusLine({ name: 'Alice', mode: 'manual_flush', pending: 3, cols: 120 }); expect(out).toContain('drive Alice'); expect(out).toContain('delivery=manual_flush'); expect(out).toContain('pending=3'); @@ -600,6 +606,50 @@ describe('renderStatusLine', () => { }); expect(out).toContain('\x1b[50;1H'); }); + + // A status line wider than the pane wraps past the bottom row, which scrolls + // the screen; because the line is painted ON the bottom row, every repaint + // then scrolls again, stacking old status lines into the scrollback and + // eating the agent's output one row at a time. + it('truncates the label to the terminal width so it can never wrap', () => { + const cols = 66; + const out = renderStatusLine({ + name: 'Gamemaster', + mode: 'manual_flush', + pending: 0, + rows: 24, + cols, + }); + const text = stripStatusLineAnsi(out); + expect(text.length).toBeLessThanOrEqual(cols); + // Middle-truncated: the verb + agent name and the key hints both survive. + expect(text).toContain('[drive Gamemaster'); + expect(text).toContain('Ctrl+C detach]'); + expect(text).toContain('…'); + }); + + it('leaves a label that already fits untouched', () => { + const out = renderStatusLine({ + name: 'Gamemaster', + mode: 'manual_flush', + pending: 0, + rows: 24, + cols: 120, + }); + const text = stripStatusLineAnsi(out); + expect(text).toBe('[drive Gamemaster | delivery=manual_flush | pending=0 | Ctrl+] deliver | Ctrl+C detach]'); + expect(text).not.toContain('…'); + }); + + // `drive` only paints when the local size is known, so this is the + // degenerate path — but assuming a terminal *wider* than 80 would + // reintroduce the wrap on the most common default width. + it('assumes 80 columns when the width is unknown', () => { + const text = stripStatusLineAnsi( + renderStatusLine({ name: 'Gamemaster', mode: 'manual_flush', pending: 0, rows: 24 }) + ); + expect(text.length).toBeLessThanOrEqual(80); + }); }); describe('runDriveSession', () => { diff --git a/packages/cli/src/cli/lib/attach-drive.ts b/packages/cli/src/cli/lib/attach-drive.ts index ca6133001..eee4860bf 100644 --- a/packages/cli/src/cli/lib/attach-drive.ts +++ b/packages/cli/src/cli/lib/attach-drive.ts @@ -52,8 +52,10 @@ import WebSocket from 'ws'; import { captureAndRenderSnapshot, + clampStatusLineText, createBackpressureAwareWriter, DETACH_CLEANUP_DEADLINE_MS, + pickInitialTerminalCols, pickInitialTerminalRows, prepareAttachTarget, resetLocalTerminalOnDetach, @@ -543,6 +545,8 @@ export function renderStatusLine(opts: { pending: number; /** Terminal rows — defaults to 24 if unknown. The status line lands on row N. */ rows?: number; + /** Terminal columns — the label is truncated to fit. Defaults to 80. */ + cols?: number; }): string { const row = Math.max(opts.rows ?? 24, 1); // The Ctrl+] hint names the action the NEXT press performs: in manual_flush @@ -550,7 +554,10 @@ export function renderStatusLine(opts: { // re-holds. Without the hint, a parked message is invisible beyond the // pending counter and a driven agent looks like it never receives replies. const toggleHint = opts.mode === 'manual_flush' ? 'Ctrl+] deliver' : 'Ctrl+] hold'; - const text = `[drive ${opts.name} | delivery=${opts.mode} | pending=${opts.pending} | ${toggleHint} | Ctrl+C detach]`; + const text = clampStatusLineText( + `[drive ${opts.name} | delivery=${opts.mode} | pending=${opts.pending} | ${toggleHint} | Ctrl+C detach]`, + opts.cols + ); // ESC 7 = save cursor; ESC[;1H = move to bottom row; ESC[2K = clear line; // ESC[7m = reverse video; ESC[0m = reset; ESC 8 = restore cursor. return `\x1b7\x1b[${row};1H\x1b[2K\x1b[7m${text}\x1b[0m\x1b8`; @@ -637,6 +644,7 @@ function runDriveSessionLoop(state: DriveSessionState, deps: DriveDependencies): let currentMode: InboundDeliveryMode = 'manual_flush'; let currentRevision = state.sessionRevision; let terminalRows = pickInitialTerminalRows(state.initialLocalSize, undefined); + let terminalCols = pickInitialTerminalCols(state.initialLocalSize, undefined); const parser = new KeybindParser(); // Stateful UTF-8 decoder for forwarded stdin. Decoding each raw stdin chunk // independently would turn a multi-byte character split across `data` @@ -682,7 +690,8 @@ function runDriveSessionLoop(state: DriveSessionState, deps: DriveDependencies): // is mid escape-sequence (no splicing into a half-sent CSI), rate-limits // per-chunk repaints, and skips painting entirely on a non-TTY stdout. const statusController = new StatusLineController({ - render: () => renderStatusLine({ name, mode: currentMode, pending, rows: terminalRows }), + render: () => + renderStatusLine({ name, mode: currentMode, pending, rows: terminalRows, cols: terminalCols }), write: deps.writeChunk, enabled: statusLineEnabled, coalesceMs: deps.statusRepaintCoalesceMs ?? 40, @@ -714,6 +723,7 @@ function runDriveSessionLoop(state: DriveSessionState, deps: DriveDependencies): const size = deps.terminal.getSize(); if (!size) return; terminalRows = size.rows; + terminalCols = size.cols; predictiveEcho?.onResize(size.cols, size.rows); trackResize( resizeWorker(connection, name, size.rows, size.cols, deps.fetch, { @@ -1094,6 +1104,7 @@ function runDriveSessionLoop(state: DriveSessionState, deps: DriveDependencies): if (settled) return; } terminalRows = pickInitialTerminalRows(state.initialLocalSize, snapshot.rows); + terminalCols = pickInitialTerminalCols(state.initialLocalSize, snapshot.cols); // Track the snapshot bytes for boundary state before the first repaint. statusController.observeOutput(snapshotBytes); paintStatus(); diff --git a/packages/cli/src/cli/lib/attach-passthrough.test.ts b/packages/cli/src/cli/lib/attach-passthrough.test.ts index a666a145e..4772a2a69 100644 --- a/packages/cli/src/cli/lib/attach-passthrough.test.ts +++ b/packages/cli/src/cli/lib/attach-passthrough.test.ts @@ -495,6 +495,18 @@ describe('renderStatusLine', () => { expect(out).toContain('\x1b[7m'); expect(out).toContain('\x1b[0m'); }); + + // Same wrap-then-scroll cascade `drive` hits: a label wider than the pane + // scrolls the screen on every repaint and shreds the agent's TUI behind it. + it('truncates the label to the terminal width so it can never wrap', () => { + const cols = 40; + // eslint-disable-next-line no-control-regex -- matching the raw ESC bytes this module emits + const strip = (s: string) => s.replace(/\x1b(?:[78]|\[[0-9;]*[A-Za-z])/g, ''); + const text = strip(renderStatusLine({ name: 'Gamemaster', mode: 'auto_inject', rows: 24, cols })); + expect(text.length).toBeLessThanOrEqual(cols); + expect(text).toContain('[passthrough'); + expect(text).toContain('detach]'); + }); }); describe('runPassthroughSession', () => { diff --git a/packages/cli/src/cli/lib/attach-passthrough.ts b/packages/cli/src/cli/lib/attach-passthrough.ts index f998022f7..4364ce80c 100644 --- a/packages/cli/src/cli/lib/attach-passthrough.ts +++ b/packages/cli/src/cli/lib/attach-passthrough.ts @@ -31,8 +31,10 @@ import WebSocket from 'ws'; import { captureAndRenderSnapshot, + clampStatusLineText, createBackpressureAwareWriter, DETACH_CLEANUP_DEADLINE_MS, + pickInitialTerminalCols, pickInitialTerminalRows, prepareAttachTarget, resetLocalTerminalOnDetach, @@ -266,9 +268,18 @@ export class PassthroughKeybindParser { * save/restore-cursor trick as `drive`, no pending counter (there * isn't one in passthrough session). */ -export function renderStatusLine(opts: { name: string; mode: InboundDeliveryMode; rows?: number }): string { +export function renderStatusLine(opts: { + name: string; + mode: InboundDeliveryMode; + rows?: number; + /** Terminal columns — the label is truncated to fit. Defaults to 80. */ + cols?: number; +}): string { const row = Math.max(opts.rows ?? 24, 1); - const text = `[passthrough ${opts.name} | delivery=${opts.mode} | Ctrl+C detach]`; + const text = clampStatusLineText( + `[passthrough ${opts.name} | delivery=${opts.mode} | Ctrl+C detach]`, + opts.cols + ); return `\x1b7\x1b[${row};1H\x1b[2K\x1b[7m${text}\x1b[0m\x1b8`; } @@ -380,6 +391,7 @@ export async function runPassthroughSession( // output around attach time). See StreamSyncBuffer. const sync = new StreamSyncBuffer(); let terminalRows = pickInitialTerminalRows(initialLocalSize, undefined); + let terminalCols = pickInitialTerminalCols(initialLocalSize, undefined); // Adaptive predictive echo masks round-trip latency on remote brokers. // Seeded with the snapshot (after it is painted) so its confirmed model @@ -407,7 +419,8 @@ export async function runPassthroughSession( // Boundary-held + coalesced status painter (skips non-TTY stdout). const statusController = new StatusLineController({ - render: () => renderStatusLine({ name, mode: 'auto_inject', rows: terminalRows }), + render: () => + renderStatusLine({ name, mode: 'auto_inject', rows: terminalRows, cols: terminalCols }), write: deps.writeChunk, enabled: statusLineEnabled, coalesceMs: deps.statusRepaintCoalesceMs ?? 40, @@ -432,6 +445,7 @@ export async function runPassthroughSession( const size = deps.terminal.getSize(); if (!size) return; terminalRows = size.rows; + terminalCols = size.cols; predictiveEcho?.onResize(size.cols, size.rows); trackResize( resizeWorker(connection, name, size.rows, size.cols, deps.fetch, { @@ -724,6 +738,7 @@ export async function runPassthroughSession( if (settled) return; } terminalRows = pickInitialTerminalRows(initialLocalSize, snapshot.rows); + terminalCols = pickInitialTerminalCols(initialLocalSize, snapshot.cols); // Track the snapshot bytes for boundary state before the first repaint. statusController.observeOutput(snapshotBytes); paintStatus(); diff --git a/packages/cli/src/cli/lib/attach.test.ts b/packages/cli/src/cli/lib/attach.test.ts index e6ee900c5..16e4ee39e 100644 --- a/packages/cli/src/cli/lib/attach.test.ts +++ b/packages/cli/src/cli/lib/attach.test.ts @@ -3,7 +3,10 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { AnsiBoundaryScanner, captureAndRenderSnapshot, + clampStatusLineText, createBackpressureAwareWriter, + DEFAULT_STATUS_LINE_COLS, + pickInitialTerminalCols, LOCAL_TERMINAL_RESET_SEQUENCE, resetLocalTerminalOnDetach, restoreInboundDeliveryModeOnDetach, @@ -787,3 +790,47 @@ describe('createBackpressureAwareWriter', () => { expect(written).toEqual(['a']); }); }); + +describe('pickInitialTerminalCols', () => { + it('prefers the local terminal width over the agent PTY width', () => { + expect(pickInitialTerminalCols({ rows: 24, cols: 66 }, 200)).toBe(66); + }); + + it('falls back to the snapshot width, then to undefined', () => { + expect(pickInitialTerminalCols(null, 200)).toBe(200); + expect(pickInitialTerminalCols(null, 0)).toBeUndefined(); + expect(pickInitialTerminalCols(null, undefined)).toBeUndefined(); + }); +}); + +describe('clampStatusLineText', () => { + const label = '[drive Gamemaster | delivery=manual_flush | pending=0 | Ctrl+] deliver | Ctrl+C detach]'; + + it('passes a label that already fits through untouched', () => { + expect(clampStatusLineText(label, 120)).toBe(label); + expect(clampStatusLineText(label, label.length)).toBe(label); + }); + + it('never returns more characters than the terminal is wide', () => { + for (const cols of [2, 10, 40, 66, 80, 86]) { + expect(clampStatusLineText(label, cols).length).toBeLessThanOrEqual(cols); + } + }); + + it('keeps the head and the tail so the verb and the key hints both survive', () => { + const out = clampStatusLineText(label, 66); + expect(out.startsWith('[drive Gamemaster')).toBe(true); + expect(out.endsWith('Ctrl+C detach]')).toBe(true); + expect(out).toContain('\u2026'); + }); + + it('assumes 80 columns when the width is unknown', () => { + expect(clampStatusLineText(label, undefined).length).toBeLessThanOrEqual(DEFAULT_STATUS_LINE_COLS); + expect(clampStatusLineText(label, 0).length).toBeLessThanOrEqual(DEFAULT_STATUS_LINE_COLS); + }); + + it('degrades to a bare head rather than wrapping on a pathological width', () => { + expect(clampStatusLineText(label, 1)).toBe('['); + expect(clampStatusLineText(label, 0).length).toBeLessThanOrEqual(DEFAULT_STATUS_LINE_COLS); + }); +}); diff --git a/packages/cli/src/cli/lib/attach.ts b/packages/cli/src/cli/lib/attach.ts index f63c57457..65ef98507 100644 --- a/packages/cli/src/cli/lib/attach.ts +++ b/packages/cli/src/cli/lib/attach.ts @@ -359,6 +359,55 @@ export function pickInitialTerminalRows( return undefined; } +/** + * Pick the status-line width. Same precedence as + * {@link pickInitialTerminalRows} — the LOCAL terminal wins, because the + * status line has to fit the pane the human is looking at, not the agent's + * PTY. + */ +export function pickInitialTerminalCols( + localSize: { rows: number; cols: number } | null, + snapshotCols: number | undefined +): number | undefined { + if (localSize) return localSize.cols; + if (typeof snapshotCols === 'number' && snapshotCols > 0) return snapshotCols; + return undefined; +} + +/** Width assumed when the local terminal never reported one. */ +export const DEFAULT_STATUS_LINE_COLS = 80; + +/** + * Clamp a status-line label to the terminal width. + * + * The interactive verbs paint their status line at the bottom row inside an + * `ESC 7` / `ESC 8` (DECSC/DECRC) pair. That is only safe while the text + * *fits*: a label wider than the pane wraps past the last row, which scrolls + * the screen. Because the label is painted on the bottom row, every repaint + * then scrolls again — promoting the previous status line into the scrollback + * as content and eating one row of the agent's output each time. A narrow + * pane therefore turns a single status line into a growing stack of them with + * the agent's TUI shredded behind it (#1360 follow-up; reproduced at 66 + * columns, where an 87-column `drive` label cost six rows of agent output). + * + * Truncation keeps the paint inside one row, so the wrap — and the scroll + * cascade it triggers — can never happen. The tail is the part that carries + * the key hints, so an over-long label is trimmed from the *middle*: the verb + * and agent name stay readable and the `Ctrl+…` hints survive. + */ +export function clampStatusLineText(text: string, cols: number | undefined): string { + const width = typeof cols === 'number' && cols > 0 ? Math.floor(cols) : DEFAULT_STATUS_LINE_COLS; + if (text.length <= width) return text; + // Too narrow to say anything useful — a bare head is still better than a + // wrap, and `…` alone would be meaningless. + if (width <= 1) return text.slice(0, Math.max(width, 0)); + const ellipsis = '…'; + const keep = width - ellipsis.length; + const tail = Math.floor(keep / 2); + const head = keep - tail; + return `${text.slice(0, head)}${ellipsis}${tail > 0 ? text.slice(text.length - tail) : ''}`; +} + /** * Sync the agent's PTY to the driver's local terminal size. tmux / * screen / ssh all do this — without it a TUI in the agent renders into diff --git a/tests/e2e/tic-tac-toe/README.md b/tests/e2e/tic-tac-toe/README.md new file mode 100644 index 000000000..4ba32a455 --- /dev/null +++ b/tests/e2e/tic-tac-toe/README.md @@ -0,0 +1,93 @@ +# Three-PTY tic-tac-toe E2E + +Drives the attach clients (`view` / `drive` / `passthrough`) the way a human +actually uses them — **through a real PTY** — and plays a real game of +tic-tac-toe between three PTY agents over the relay protocol. + +The suite has two halves, split so the cheap one always runs. + +## 1. Status-line rendering (always runs, no stack, no LLM) + +Replays the real `renderStatusLine` output through a headless terminal +emulator (`@xterm/headless`) and asserts the agent's screen survives. + +This is the regression guard for the bug that made a `drive` pane unreadable: + +> The status line is painted on the bottom row inside an `ESC 7` / `ESC 8` +> (DECSC/DECRC) pair. That is only safe while the label *fits*. A label wider +> than the pane wraps past the last row, which scrolls the screen — and because +> the label sits ON the bottom row, the next repaint scrolls again. Old status +> lines get promoted into the scrollback as content and the agent's TUI loses a +> row of output per repaint. + +The `drive` label is 87 columns wide, so this fired on a **standard 80-column +terminal**, not just narrow tmux panes. At 66 columns (a quarter-screen pane) six +repaints cost six rows of agent output and left six stacked status bars behind. + +Byte-level assertions cannot catch this — "painted once" and "painted six times +while scrolling the screen away" are the same bytes on the wire. Only replaying +into an emulator sees what the human sees. + +## 2. The live game (opt-in) + +Three `claude` PTY agents — `Gamemaster`, `PlayerA`, `PlayerB` — play a full +game. The Gamemaster owns the board and DMs each player in turn; the players +reply `MOVE `. A `view` client watches each agent through its own PTY. + +Asserts: + +| Assertion | Guards | +| ---------------------- | ---------------------------------------------------------------------------- | +| view streams grow | a `view` pane frozen on its attach snapshot (live `worker_stream` never lands) | +| relay traffic both ways | agents actually using the relay protocol rather than talking to themselves | +| `GAME OVER` reaches both players | the round trip completes, not just the first hop | +| every pane renders coherently, no row wider than the pane | wrap/scroll corruption in the live stream | + +### Running it + +```bash +npm run build:core # relay CLI + packages +cargo build --release --bin agent-relay-broker # broker + +RELAY_TTT_LIVE=1 \ +RELAYCAST_ENGINE_DIR=/path/to/relaycast \ # a built relaycast checkout + npm run test:e2e -- tests/e2e/tic-tac-toe +``` + +Without `RELAY_TTT_LIVE=1` the live half skips; when the engine or broker +binary is missing it skips too (never fails), same convention as the fleet E2E. + +The game is three LLM agents taking turns over the network — budget ~3-5 +minutes of wall clock for it to reach `GAME OVER`. + +## Gotchas this suite encodes + +These each cost real debugging time; they are handled in `harness.ts` so the +next person doesn't rediscover them. + +- **A fresh engine DB per run is mandatory.** The broker enrolls its node under + the project directory name. Re-enrolling an already-known name fails + `node_name_conflict`, after which the engine has no delivery-ready provider + for that node and defers *every* message + (`[delivery.route] provider not delivery-ready`). Relay messaging then looks + silently broken — agents stay "working" in `agent list` and never receive a + thing. + +- **The harness HOME must be past first-run onboarding.** Otherwise `claude` + opens on the theme picker and waits. The agent reports `working`, its pane + never changes, and that is indistinguishable from a dead view stream. + `seedHarnessHome` copies the caller's `~/.claude.json` and flips + `hasCompletedOnboarding`. + +- **A quiet agent is not a broken view.** An agent spawned with no task sits at + an empty prompt producing no output, so its `view` pane legitimately never + updates. The suite gives every agent a task before asserting on stream + growth. + +- **`AGENT_RELAY_MCP_COMMAND` must point at the local build.** The broker + otherwise configures spawned agents with `npx -y agent-relay mcp`, which + fetches the published package instead of the code under test. + +- **PTYs come from `pty-run.py`.** `stdout.isTTY` gates the status line, the + terminal reset on detach, and the input-report filter; driving the clients + through a pipe exercises a different path than the one a human sees. diff --git a/tests/e2e/tic-tac-toe/harness.ts b/tests/e2e/tic-tac-toe/harness.ts new file mode 100644 index 000000000..734e320f1 --- /dev/null +++ b/tests/e2e/tic-tac-toe/harness.ts @@ -0,0 +1,366 @@ +/** + * Boot helpers for the three-PTY tic-tac-toe E2E. + * + * Unlike the unit suites (fake WebSocket, injected stdout), this drives the + * attach clients through a **real PTY**. That matters: `view`, `drive`, and + * `passthrough` all gate behaviour on `stdout.isTTY` — the status line is only + * painted on a TTY, and the whole class of bug this suite guards (a status line + * wider than the pane wrapping, scrolling the screen, and stacking into the + * agent's output) is invisible through a pipe. + */ + +import { spawn, type ChildProcess } from 'node:child_process'; +import { existsSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +export const REPO_ROOT = path.resolve(HERE, '..', '..', '..'); +export const PTY_RUN = path.join(HERE, 'pty-run.py'); +export const RELAY_CLI = path.join(REPO_ROOT, 'packages', 'cli', 'dist', 'cli', 'index.js'); + +/** Locate a built relaycast engine `serve` bin (same lookup as the fleet E2E). */ +export function resolveEngineServe(): string | null { + const candidates = [ + process.env.RELAYCAST_ENGINE_DIR + ? path.join(process.env.RELAYCAST_ENGINE_DIR, 'packages', 'engine', 'dist', 'bin', 'serve.js') + : null, + path.resolve(REPO_ROOT, '..', 'relaycast', 'packages', 'engine', 'dist', 'bin', 'serve.js'), + ].filter((p): p is string => p !== null); + return candidates.find((p) => existsSync(p)) ?? null; +} + +export function resolveBrokerBinary(): string | null { + const candidates = [ + process.env.BROKER_BINARY_PATH ?? null, + path.join(REPO_ROOT, 'target', 'release', 'agent-relay-broker'), + ].filter((p): p is string => p !== null); + return candidates.find((p) => existsSync(p)) ?? null; +} + +export interface Preflight { + ok: boolean; + reason: string; + engineServe?: string; + brokerBinary?: string; +} + +/** + * Check the prerequisites. Like the fleet E2E, the suite **skips cleanly** + * rather than failing when a local relaycast engine or broker binary is + * missing — the default `npm test` must not require them. + */ +export function preflight(): Preflight { + if (!existsSync(RELAY_CLI)) { + return { ok: false, reason: 'relay CLI not built; run `npm run build:core`' }; + } + if (!hasPython3()) { + return { ok: false, reason: 'python3 not found; it allocates the PTYs this suite drives' }; + } + const engineServe = resolveEngineServe(); + if (!engineServe) { + return { + ok: false, + reason: 'relaycast engine serve bin not found; set RELAYCAST_ENGINE_DIR to a built checkout', + }; + } + const brokerBinary = resolveBrokerBinary(); + if (!brokerBinary) { + return { ok: false, reason: 'broker binary not found; run `cargo build --release`' }; + } + return { ok: true, reason: 'ok', engineServe, brokerBinary }; +} + +function hasPython3(): boolean { + const probe = spawnSyncQuiet('python3', ['-c', 'import pty']); + return probe === 0; +} + +function spawnSyncQuiet(cmd: string, args: string[]): number { + // eslint-disable-next-line @typescript-eslint/no-var-requires -- sync probe only + const { spawnSync } = require('node:child_process') as typeof import('node:child_process'); + const res = spawnSync(cmd, args, { stdio: 'ignore' }); + return res.status ?? 1; +} + +/** + * Strip ambient `RELAY_*` / `AGENT_RELAY_*` config so a developer's real + * workspace is never joined by a test broker, and point the broker at the + * local engine. + */ +export function cleanEnv(extra: Record): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = {}; + for (const [key, value] of Object.entries(process.env)) { + if (key.startsWith('RELAY_') || key.startsWith('AGENT_RELAY_') || key.startsWith('RELAYCAST_')) { + continue; + } + env[key] = value; + } + return { ...env, ...extra }; +} + +export async function getFreePort(): Promise { + const { createServer } = await import('node:net'); + return new Promise((resolve, reject) => { + const server = createServer(); + server.once('error', reject); + server.listen(0, '127.0.0.1', () => { + const address = server.address(); + const port = typeof address === 'object' && address ? address.port : 0; + server.close(() => resolve(port)); + }); + }); +} + +export async function waitFor( + check: () => Promise | boolean, + opts: { timeoutMs: number; label: string; intervalMs?: number } +): Promise { + const deadline = Date.now() + opts.timeoutMs; + for (;;) { + if (await check()) return; + if (Date.now() > deadline) throw new Error(`timed out waiting for ${opts.label}`); + await new Promise((r) => setTimeout(r, opts.intervalMs ?? 250)); + } +} + +export interface EngineHandle { + baseUrl: string; + stop(): void; +} + +export async function startEngine(serveBin: string, tmpRoot: string): Promise { + const port = await getFreePort(); + const baseUrl = `http://127.0.0.1:${port}`; + const child = spawn( + process.execPath, + [serveBin, '--port', String(port), '--db', path.join(tmpRoot, 'relaycast.db'), '--env', 'test'], + { env: cleanEnv({ HOME: tmpRoot }), stdio: ['ignore', 'pipe', 'pipe'] } + ); + child.stdout?.on('data', () => {}); + child.stderr?.on('data', () => {}); + + await waitFor( + async () => { + try { + return (await fetch(`${baseUrl}/`)).status > 0; + } catch { + return false; + } + }, + { timeoutMs: 30_000, label: 'relaycast engine ready' } + ); + + return { + baseUrl, + stop() { + child.kill('SIGKILL'); + }, + }; +} + +export interface BrokerHandle { + url: string; + apiKey: string; + /** POST /api/send — publish a relay message as `from` to `to`. */ + send(to: string, from: string, text: string): Promise; + /** GET the agent's rendered screen. */ + snapshot(name: string): Promise<{ screen?: string; rows?: number; cols?: number }>; + spawnAgent(name: string, task: string): Promise; + stop(): void; +} + +/** + * Start a broker in `projectDir` pointed at the local engine. + * + * NOTE: each run needs a **fresh** engine DB. The broker enrolls its node under + * the project directory name, and re-enrolling an already-known name fails with + * `node_name_conflict` — after which the engine has no delivery-ready provider + * for the node and defers every message ("provider not delivery-ready"), which + * looks exactly like relay messaging being silently broken. + */ +export async function startBroker( + projectDir: string, + engineBaseUrl: string, + home: string +): Promise { + const env = cleanEnv({ + HOME: home, + RELAYCAST_BASE_URL: engineBaseUrl, + BROKER_BINARY_PATH: resolveBrokerBinary() ?? '', + // Point spawned agents at the freshly built MCP server rather than + // `npx -y agent-relay mcp`, which would fetch the published package. + AGENT_RELAY_MCP_COMMAND: `node ${path.join(REPO_ROOT, 'packages', 'cli', 'dist', 'cli', 'agent-relay-mcp.js')}`, + }); + + await runCli(['node', 'up', '--background'], projectDir, env); + + const connectionPath = path.join(projectDir, '.agentworkforce', 'relay', 'connection.json'); + await waitFor(() => existsSync(connectionPath), { + timeoutMs: 30_000, + label: 'broker connection.json', + }); + const connection = JSON.parse(readFileSync(connectionPath, 'utf-8')) as { + url: string; + api_key: string; + }; + + const headers = { 'X-API-Key': connection.api_key, 'Content-Type': 'application/json' }; + + return { + url: connection.url, + apiKey: connection.api_key, + send: (to, from, text) => + fetch(`${connection.url}/api/send`, { + method: 'POST', + headers, + body: JSON.stringify({ to, from, text }), + }), + async snapshot(name) { + const res = await fetch(`${connection.url}/api/spawned/${encodeURIComponent(name)}/snapshot`, { + headers: { 'X-API-Key': connection.api_key }, + }); + return (await res.json()) as { screen?: string; rows?: number; cols?: number }; + }, + async spawnAgent(name, task) { + await runCli(['node', 'agent', 'spawn', 'claude', `--name=${name}`, '--task', task], projectDir, env); + }, + stop() { + try { + spawn(process.execPath, [RELAY_CLI, 'node', 'down', '--force'], { + cwd: projectDir, + env, + stdio: 'ignore', + }).unref(); + } catch { + // best effort + } + }, + }; +} + +function runCli(args: string[], cwd: string, env: NodeJS.ProcessEnv): Promise { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [RELAY_CLI, ...args], { cwd, env, stdio: ['ignore', 'pipe', 'pipe'] }); + let out = ''; + child.stdout?.on('data', (d) => (out += String(d))); + child.stderr?.on('data', (d) => (out += String(d))); + child.on('error', reject); + child.on('exit', (code) => + code === 0 ? resolve(out) : reject(new Error(`relay ${args.join(' ')} exited ${code}: ${out}`)) + ); + }); +} + +/** An attach client running inside a real PTY, capturing raw bytes. */ +export interface PtyClient { + capturePath: string; + size(): number; + /** Raw captured bytes, exactly as the terminal received them. */ + read(): Buffer; + /** Send keystrokes to the client (e.g. `\x03` to detach). */ + type(data: string): void; + stop(): void; + readonly child: ChildProcess; +} + +/** Attach to `name` in `mode` inside a real PTY of the given size. */ +export function attachInPty(opts: { + name: string; + mode: 'view' | 'drive' | 'passthrough'; + cols: number; + rows: number; + capturePath: string; + projectDir: string; + env: NodeJS.ProcessEnv; +}): PtyClient { + const child = spawn( + 'python3', + [ + PTY_RUN, + '--out', + opts.capturePath, + '--cols', + String(opts.cols), + '--rows', + String(opts.rows), + '--', + process.execPath, + RELAY_CLI, + 'node', + 'agent', + 'attach', + opts.name, + '--mode', + opts.mode, + ], + { cwd: opts.projectDir, env: opts.env, stdio: ['pipe', 'pipe', 'pipe'] } + ); + child.stdout?.on('data', () => {}); + child.stderr?.on('data', () => {}); + + return { + capturePath: opts.capturePath, + child, + size: () => (existsSync(opts.capturePath) ? statSync(opts.capturePath).size : 0), + read: () => (existsSync(opts.capturePath) ? readFileSync(opts.capturePath) : Buffer.alloc(0)), + type: (data) => child.stdin?.write(data), + stop: () => child.kill('SIGKILL'), + }; +} + +/** + * Replay a raw capture through a headless terminal emulator and return the + * visible screen, one string per row. + * + * Byte-level assertions cannot tell "the status line was painted once" from + * "it was painted six times and scrolled the agent's output away" — both are + * the same bytes on the wire. Only an emulator sees what the human sees. + */ +export async function renderScreen( + capture: Buffer, + cols: number, + rows: number +): Promise { + const { Terminal } = await import('@xterm/headless'); + const term = new Terminal({ cols, rows, allowProposedApi: true }); + await new Promise((resolve) => term.write(new Uint8Array(capture), resolve)); + const buf = term.buffer.active; + const out: string[] = []; + for (let y = 0; y < rows; y += 1) { + out.push((buf.getLine(y)?.translateToString(true) ?? '').trimEnd()); + } + return out; +} + +/** + * Replay a capture and return everything the pane ever showed — scrollback + * included — as one string. + * + * Content assertions must run on this, never on the raw capture. A TUI paints + * with absolute cursor addressing, so a phrase the human plainly reads + * ("Relay message from PlayerA") is generally *not* a contiguous byte run in + * the stream: it arrives interleaved with positioning and colour sequences. + * Only after an emulator lays it back out on a grid does it become searchable + * text. + */ +export async function renderTranscript( + capture: Buffer, + cols: number, + rows: number, + scrollback = 20_000 +): Promise { + const { Terminal } = await import('@xterm/headless'); + const term = new Terminal({ cols, rows, scrollback, allowProposedApi: true }); + await new Promise((resolve) => term.write(new Uint8Array(capture), resolve)); + const lines: string[] = []; + // Both buffers matter: a full-screen TUI lives in the alternate buffer while + // the pre-alt-screen output (and anything printed after it exits) is in the + // normal one. + for (const buf of [term.buffer.normal, term.buffer.active]) { + for (let y = 0; y < buf.length; y += 1) { + lines.push(buf.getLine(y)?.translateToString(true).trimEnd() ?? ''); + } + } + return lines.join('\n'); +} diff --git a/tests/e2e/tic-tac-toe/pty-run.py b/tests/e2e/tic-tac-toe/pty-run.py new file mode 100644 index 000000000..d37006fe4 --- /dev/null +++ b/tests/e2e/tic-tac-toe/pty-run.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +"""Run a command under a real PTY and tee its raw output to a file. + +The attach clients (`view` / `drive`) behave differently when stdout is a TTY: +the status line is only painted on a TTY, the input-report filter only matters +when a real terminal would act on the reports, and `resetLocalTerminalOnDetach` +is TTY-gated. Driving them through a pipe therefore exercises a different code +path than the one a human sees, which is exactly the path the tic-tac-toe E2E +needs to assert on. + +Usage: + pty-run.py --out capture.bin [--cols 120] [--rows 40] -- cmd [args...] + +Bytes written to this process's stdin are forwarded to the child PTY, so the +orchestrator can send keystrokes (Ctrl+C to detach, Ctrl+] to toggle delivery). +Every byte the child writes is appended to `--out` verbatim — no decoding, no +newline translation — so a terminal emulator can replay it faithfully. +""" + +import argparse +import errno +import fcntl +import os +import pty +import select +import signal +import struct +import sys +import termios + + +def set_winsize(fd: int, rows: int, cols: int) -> None: + fcntl.ioctl(fd, termios.TIOCSWINSZ, struct.pack("HHHH", rows, cols, 0, 0)) + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument("--out", required=True) + parser.add_argument("--cols", type=int, default=120) + parser.add_argument("--rows", type=int, default=40) + parser.add_argument("cmd", nargs=argparse.REMAINDER) + args = parser.parse_args() + + cmd = args.cmd[1:] if args.cmd and args.cmd[0] == "--" else args.cmd + if not cmd: + print("pty-run: no command given", file=sys.stderr) + return 2 + + pid, master = pty.fork() + if pid == 0: + # Child: a fresh session already owns the slave pty as its controlling + # terminal. Advertise a capable terminal so TUIs emit the full escape + # repertoire the filters are supposed to handle. + os.environ["TERM"] = os.environ.get("TERM", "xterm-256color") + os.environ["COLUMNS"] = str(args.cols) + os.environ["LINES"] = str(args.rows) + try: + os.execvp(cmd[0], cmd) + except OSError as exc: + print(f"pty-run: exec {cmd[0]}: {exc}", file=sys.stderr) + os._exit(127) + + set_winsize(master, args.rows, args.cols) + + out = open(args.out, "wb", buffering=0) + stdin_fd = sys.stdin.fileno() + stdin_open = True + exit_code = 0 + + try: + while True: + watch = [master] + ([stdin_fd] if stdin_open else []) + try: + readable, _, _ = select.select(watch, [], [], 0.25) + except InterruptedError: + continue + + if master in readable: + try: + data = os.read(master, 65536) + except OSError as exc: + # EIO is the normal "child closed the slave side" signal. + if exc.errno != errno.EIO: + raise + data = b"" + if not data: + break + out.write(data) + + if stdin_open and stdin_fd in readable: + try: + keys = os.read(stdin_fd, 65536) + except OSError: + keys = b"" + if not keys: + stdin_open = False + else: + os.write(master, keys) + + # Reap without blocking so a child that exits while we still have + # buffered output does not strand the loop. + waited, status = os.waitpid(pid, os.WNOHANG) + if waited == pid: + # Drain whatever is still sitting in the pty buffer. + while True: + try: + rest = os.read(master, 65536) + except OSError: + break + if not rest: + break + out.write(rest) + exit_code = ( + os.WEXITSTATUS(status) + if os.WIFEXITED(status) + else 128 + os.WTERMSIG(status) + ) + pid = 0 + break + finally: + out.close() + if pid: + try: + os.kill(pid, signal.SIGTERM) + os.waitpid(pid, 0) + except OSError: + pass + try: + os.close(master) + except OSError: + pass + + return exit_code + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts new file mode 100644 index 000000000..22d872253 --- /dev/null +++ b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts @@ -0,0 +1,298 @@ +/** + * Three-PTY tic-tac-toe E2E. + * + * Two independent concerns, split so the cheap one always runs: + * + * 1. **Status-line rendering** (no stack, no LLM, no network) — replays the + * real `renderStatusLine` output through a headless terminal emulator and + * asserts the agent's screen survives. This is the regression guard for + * the bug that made a `drive` pane unreadable: a status label wider than + * the pane wraps past the bottom row, which scrolls the screen; since the + * label is painted ON the bottom row, every repaint scrolls again, + * stacking old status lines into the scrollback and eating the agent's + * output a row at a time. + * + * 2. **The live game** (needs a local relaycast engine, the broker binary, + * and a working `claude` CLI) — three PTY agents play a real game of + * tic-tac-toe over the relay protocol while a `view` client watches each + * one through a real PTY. Skips cleanly when the prerequisites are + * missing, exactly like the fleet E2E. + * + * See `README.md` in this directory for how to run the live half. + */ + +import { mkdtempSync, mkdirSync, rmSync, existsSync, cpSync, readFileSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { renderStatusLine } from '../../../packages/cli/src/cli/lib/attach-drive.js'; +import { renderStatusLine as renderPassthroughStatusLine } from '../../../packages/cli/src/cli/lib/attach-passthrough.js'; +import { + attachInPty, + cleanEnv, + preflight, + renderScreen, + renderTranscript, + startBroker, + startEngine, + waitFor, + type BrokerHandle, + type EngineHandle, + type PtyClient, +} from './harness.js'; + +/** Paint an agent TUI, then repaint the status line `repaints` times. */ +async function screenAfterStatusPaints(opts: { + cols: number; + rows: number; + repaints: number; + render: (pending: number) => string; +}): Promise { + const { Terminal } = await import('@xterm/headless'); + const term = new Terminal({ cols: opts.cols, rows: opts.rows, allowProposedApi: true }); + const write = (data: string) => new Promise((r) => term.write(data, r)); + + // An agent TUI that owns the screen: alt-screen, absolute-addressed content. + await write('\x1b[?1049h\x1b[H\x1b[2J'); + for (let i = 1; i <= opts.rows - 4; i += 1) await write(`\x1b[${i};1Hagent line ${i}`); + await write(`\x1b[${opts.rows - 4};1H`); + for (let n = 0; n < opts.repaints; n += 1) await write(opts.render(n)); + + const buf = term.buffer.active; + const screen: string[] = []; + for (let y = 0; y < opts.rows; y += 1) { + screen.push((buf.getLine(y)?.translateToString(true) ?? '').trimEnd()); + } + return screen; +} + +describe('attach status line survives a narrow pane', () => { + const ROWS = 24; + const AGENT_LINES = ROWS - 4; + + // 66 columns is roughly a quarter-screen tmux pane — the shape that made a + // real `drive` session unreadable. The `drive` label is 87 columns wide. + for (const cols of [120, 80, 66, 40]) { + it(`drive: one status row and no lost agent output at ${cols} columns`, async () => { + const screen = await screenAfterStatusPaints({ + cols, + rows: ROWS, + repaints: 6, + render: (pending) => + renderStatusLine({ name: 'Gamemaster', mode: 'manual_flush', pending, rows: ROWS, cols }), + }); + + const statusRows = screen.filter((l) => l.includes('[drive Gamemaster')).length; + const agentRows = screen.filter((l) => /^agent line \d+$/.test(l)).length; + + expect(statusRows, `status line stacked at ${cols} cols:\n${screen.join('\n')}`).toBe(1); + expect(agentRows, `agent output scrolled away at ${cols} cols:\n${screen.join('\n')}`).toBe( + AGENT_LINES + ); + // It must land on the bottom row, not wherever a scroll left it. + expect(screen[ROWS - 1]).toContain('[drive Gamemaster'); + expect(screen[ROWS - 1].length).toBeLessThanOrEqual(cols); + }); + } + + it('passthrough: one status row and no lost agent output at 40 columns', async () => { + const cols = 40; + const screen = await screenAfterStatusPaints({ + cols, + rows: ROWS, + repaints: 6, + render: () => renderPassthroughStatusLine({ name: 'Gamemaster', mode: 'auto_inject', rows: ROWS, cols }), + }); + + expect(screen.filter((l) => l.includes('[passthrough')).length).toBe(1); + expect(screen.filter((l) => /^agent line \d+$/.test(l)).length).toBe(AGENT_LINES); + expect(screen[ROWS - 1].length).toBeLessThanOrEqual(cols); + }); + + it('keeps the agent name and the detach hint readable even when truncated', () => { + const line = renderStatusLine({ name: 'Gamemaster', mode: 'manual_flush', pending: 2, rows: 24, cols: 66 }); + expect(line).toContain('[drive Gamemaster'); + expect(line).toContain('Ctrl+C detach]'); + }); +}); + +// ── the live game ──────────────────────────────────────────────────────────── + +const check = preflight(); +const liveEnabled = check.ok && process.env.RELAY_TTT_LIVE === '1'; + +describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay protocol', () => { + const COLS = 100; + const ROWS = 30; + const AGENTS = ['Gamemaster', 'PlayerA', 'PlayerB'] as const; + + let tmpRoot: string; + let engine: EngineHandle; + let broker: BrokerHandle; + const clients = new Map(); + + beforeAll(async () => { + tmpRoot = mkdtempSync(path.join(os.tmpdir(), 'relay-ttt-')); + const projectDir = path.join(tmpRoot, 'proj'); + const home = path.join(tmpRoot, 'home'); + const capDir = path.join(tmpRoot, 'cap'); + for (const dir of [projectDir, home, capDir]) mkdirSync(dir, { recursive: true }); + seedHarnessHome(home); + + engine = await startEngine(check.engineServe!, tmpRoot); + broker = await startBroker(projectDir, engine.baseUrl, home); + + for (const [name, task] of Object.entries(TASKS)) { + await broker.spawnAgent(name, task); + } + + const env = cleanEnv({ HOME: home }); + for (const name of AGENTS) { + clients.set( + name, + attachInPty({ + name, + mode: 'view', + cols: COLS, + rows: ROWS, + capturePath: path.join(capDir, `${name}.bin`), + projectDir, + env, + }) + ); + } + + // The game is three LLM agents taking turns over the network; give it room. + // + // Wait on DONE_TOKEN reaching a PLAYER. The token appears only in the + // Gamemaster's prompt, so seeing it in a player's pane proves a real + // end-of-game message was delivered. Waiting on a phrase like 'GAME OVER' + // instead matches the injected task prompt itself and fires immediately. + await waitFor( + async () => { + for (const player of ['PlayerA', 'PlayerB'] as const) { + if ((await transcript(player)).includes(DONE_TOKEN)) return true; + } + return false; + }, + { timeoutMs: 8 * 60_000, label: 'the game to finish', intervalMs: 5_000 } + ); + }, 10 * 60_000); + + /** What the pane has shown so far, scrollback included. */ + const transcript = (name: string): Promise => + renderTranscript(clients.get(name)!.read(), COLS, ROWS); + + afterAll(() => { + for (const client of clients.values()) client.stop(); + broker?.stop(); + engine?.stop(); + if (tmpRoot && existsSync(tmpRoot)) rmSync(tmpRoot, { recursive: true, force: true }); + }); + + it('streams live PTY output to every view client', () => { + for (const name of AGENTS) { + // A frozen `view` pane — the original report — shows up here as a + // capture no bigger than the one-shot attach snapshot. + expect(clients.get(name)!.size(), `${name} view stream never grew past its snapshot`).toBeGreaterThan( + 20_000 + ); + } + }); + + it('carries moves between agents over the relay protocol', async () => { + const gm = await transcript('Gamemaster'); + // Both players' moves reached the Gamemaster as relay messages, and the + // Gamemaster saw them as relay traffic (not as its own typing). + expect(gm).toContain('Relay message from PlayerA'); + expect(gm).toContain('Relay message from PlayerB'); + expect(gm).toMatch(/MOVE\s*\d/); + + for (const player of ['PlayerA', 'PlayerB'] as const) { + expect(await transcript(player), `${player} never received a relay message`).toContain( + 'Relay message from Gamemaster' + ); + } + }); + + it('reaches a terminal result and tells both players', async () => { + for (const player of ['PlayerA', 'PlayerB'] as const) { + expect(await transcript(player), `${player} was never told the game ended`).toContain(DONE_TOKEN); + } + }); + + it('renders a coherent screen in each view pane', async () => { + for (const name of AGENTS) { + const screen = await renderScreen(clients.get(name)!.read(), COLS, ROWS); + const nonBlank = screen.filter((l) => l.trim().length > 0).length; + expect(nonBlank, `${name} pane rendered blank`).toBeGreaterThan(5); + // Every row must fit: a row longer than the pane means something wrapped + // and the emulator had to scroll to place it. + for (const row of screen) expect(row.length).toBeLessThanOrEqual(COLS); + } + }); +}); + +/** + * End-of-game sentinel. Named ONLY in the Gamemaster's prompt, so finding it + * in a player's pane is proof a relay message actually crossed the wire — + * unlike a natural phrase, which the injected prompt itself would match. + */ +const DONE_TOKEN = 'TICTACTOE-COMPLETE-7F3A'; + +const RULES = `You are playing a game of tic-tac-toe with other agents over Agent Relay. +Communicate ONLY by sending relay messages with the agent-relay MCP tools (send_dm). +Board cells are numbered 1-9, left-to-right, top-to-bottom. +Keep every message to one or two short lines. Never ask a clarifying question - +if something is ambiguous, pick a reasonable interpretation and continue.`; + +const TASKS: Record = { + Gamemaster: `${RULES} +You are the GAMEMASTER. You own the board; the players do not track it. +Run the whole game to completion, without waiting for any human input: +1. send_dm to PlayerA: 'Game start. You are X. Board is empty. Your move (1-9)?' +2. When PlayerA replies, apply the move, then send_dm to PlayerB the CURRENT + board as three lines of three characters plus 'You are O. Your move (1-9)?' +3. Alternate until someone wins or the board is full. +4. Reject an illegal move by asking that player again. +5. When the game ends, send_dm BOTH players a final message naming the result + whose last line is exactly ${DONE_TOKEN}, then print the final board in your + own terminal. +Start immediately.`, + PlayerA: `${RULES} +You are PLAYERA, playing X. Wait for relay messages from Gamemaster. +Every time Gamemaster asks for your move, reply with send_dm to Gamemaster in +the form 'MOVE ' plus at most one short sentence. Never message PlayerB. +Stop playing once the Gamemaster tells you the game has finished.`, + PlayerB: `${RULES} +You are PLAYERB, playing O. Wait for relay messages from Gamemaster. +Every time Gamemaster asks for your move, reply with send_dm to Gamemaster in +the form 'MOVE ' plus at most one short sentence. Never message PlayerA. +Stop playing once the Gamemaster tells you the game has finished.`, +}; + +/** + * Give the spawned agents a HOME whose harness config is already past + * first-run onboarding. Without it, `claude` opens on the theme picker and + * sits there — the agent looks "working" in `agent list` while its pane never + * changes, which is indistinguishable from a broken view stream. + */ +function seedHarnessHome(home: string): void { + const realHome = process.env.HOME; + if (!realHome) return; + const source = path.join(realHome, '.claude.json'); + if (!existsSync(source)) return; + + const target = path.join(home, '.claude.json'); + cpSync(source, target); + const claudeDir = path.join(realHome, '.claude'); + if (existsSync(claudeDir)) cpSync(claudeDir, path.join(home, '.claude'), { recursive: true }); + + const config = JSON.parse(readFileSync(target, 'utf-8')) as Record; + config.hasCompletedOnboarding = true; + config.bypassPermissionsModeAccepted = true; + config.theme ??= 'dark'; + config.projects = {}; + writeFileSync(target, JSON.stringify(config, null, 2)); +} From aac90650a1fee2380173688ce2031cbd292e5438 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 18:50:59 +0000 Subject: [PATCH 2/8] test(e2e): assert relay traffic from the broker event stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The live scenario asserted "PlayerB messaged the Gamemaster" by searching the Gamemaster's rendered pane. That cannot work: the harness TUI runs in the alternate screen buffer, which keeps no scrollback, so an earlier message is gone from the grid as soon as the pane repaints. It also searched the raw PTY capture, where absolute cursor addressing means a phrase the human plainly reads is not a contiguous byte run. Protocol claims now read the broker's `relay_inbound` stream (both directions, both players, plus at least three delivered `MOVE ` replies); visual claims still replay the capture through an emulator. Neither substitutes for the other — the status-line bug emitted perfectly well-formed frames. Also replaces the readiness wait: waiting for "GAME OVER" matched the injected task prompt itself and fired in 20s, before a game had been played. It now waits for a token named only in the Gamemaster's prompt to be *delivered* to a player. Full suite green, live game included (10/10, 211s). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- tests/e2e/tic-tac-toe/README.md | 34 ++++++-- tests/e2e/tic-tac-toe/harness.ts | 87 +++++++++++++------ tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts | 62 +++++++------ 3 files changed, 123 insertions(+), 60 deletions(-) diff --git a/tests/e2e/tic-tac-toe/README.md b/tests/e2e/tic-tac-toe/README.md index 4ba32a455..8a58c1cce 100644 --- a/tests/e2e/tic-tac-toe/README.md +++ b/tests/e2e/tic-tac-toe/README.md @@ -36,12 +36,26 @@ reply `MOVE `. A `view` client watches each agent through its own PTY. Asserts: -| Assertion | Guards | -| ---------------------- | ---------------------------------------------------------------------------- | -| view streams grow | a `view` pane frozen on its attach snapshot (live `worker_stream` never lands) | -| relay traffic both ways | agents actually using the relay protocol rather than talking to themselves | -| `GAME OVER` reaches both players | the round trip completes, not just the first hop | -| every pane renders coherently, no row wider than the pane | wrap/scroll corruption in the live stream | +| Assertion | Guards | +| --------------------------------------------------------- | ------------------------------------------------------------------------------ | +| view streams grow | a `view` pane frozen on its attach snapshot (live `worker_stream` never lands) | +| `relay_inbound` in both directions, for both players | agents actually using the relay protocol rather than talking to themselves | +| ≥3 delivered replies matching `MOVE ` | a game that "ended" without anyone actually playing | +| the end-of-game token is delivered to both players | the round trip completes, not just the first hop | +| every pane renders coherently, no row wider than the pane | wrap/scroll corruption in the live stream | + +**Where each assertion runs matters.** Protocol claims read the broker's +`relay_inbound` event stream; visual claims replay the PTY capture through an +emulator. Neither substitutes for the other: + +- A rendered pane cannot prove a message arrived. A TUI paints with absolute + cursor addressing, so a phrase the human plainly reads is not a contiguous + byte run in the capture — and a full-screen harness lives in the alternate + screen buffer, which keeps **no scrollback**, so once it repaints the earlier + message is gone from the grid entirely. +- The event stream cannot prove anything rendered correctly. The status-line + bug emitted perfectly well-formed frames; only the emulator sees that they + landed on top of each other. ### Running it @@ -58,7 +72,7 @@ Without `RELAY_TTT_LIVE=1` the live half skips; when the engine or broker binary is missing it skips too (never fails), same convention as the fleet E2E. The game is three LLM agents taking turns over the network — budget ~3-5 -minutes of wall clock for it to reach `GAME OVER`. +minutes of wall clock for it to finish (a passing run took 211s). ## Gotchas this suite encodes @@ -91,3 +105,9 @@ next person doesn't rediscover them. - **PTYs come from `pty-run.py`.** `stdout.isTTY` gates the status line, the terminal reset on detach, and the input-report filter; driving the clients through a pipe exercises a different path than the one a human sees. + +- **Don't wait on a natural phrase.** The task prompt is injected into the + agent's PTY, so waiting for "GAME OVER" to appear matches the prompt itself + and fires instantly. The suite waits on `DONE_TOKEN`, which only the + Gamemaster's prompt names — seeing it *delivered to a player* is proof the + game really ended. diff --git a/tests/e2e/tic-tac-toe/harness.ts b/tests/e2e/tic-tac-toe/harness.ts index 734e320f1..496d4f082 100644 --- a/tests/e2e/tic-tac-toe/harness.ts +++ b/tests/e2e/tic-tac-toe/harness.ts @@ -333,34 +333,69 @@ export async function renderScreen( return out; } +/** One relay message the broker actually delivered. */ +export interface RelayMessage { + from: string; + target: string; + body: string; +} + +export interface EventRecorder { + /** Every delivered chat message, in arrival order. */ + messages(): RelayMessage[]; + /** Frame counts by `kind` — `worker_stream` here is live PTY output. */ + kinds(): Record; + stop(): void; +} + /** - * Replay a capture and return everything the pane ever showed — scrollback - * included — as one string. + * Record the broker's event stream for the life of a scenario. + * + * Assertions about *protocol* traffic belong here, not on a rendered pane. Two + * reasons a terminal replay cannot answer "did this message ever arrive?": + * a TUI paints with absolute cursor addressing, so a phrase the human plainly + * reads is not a contiguous byte run in the stream; and a full-screen harness + * lives in the alternate screen buffer, which keeps no scrollback — once it + * repaints, the earlier message is genuinely gone from the grid. * - * Content assertions must run on this, never on the raw capture. A TUI paints - * with absolute cursor addressing, so a phrase the human plainly reads - * ("Relay message from PlayerA") is generally *not* a contiguous byte run in - * the stream: it arrives interleaved with positioning and colour sequences. - * Only after an emulator lays it back out on a grid does it become searchable - * text. + * The panes are still the right place to assert how things *look*; this is the + * right place to assert what was *delivered*. */ -export async function renderTranscript( - capture: Buffer, - cols: number, - rows: number, - scrollback = 20_000 -): Promise { - const { Terminal } = await import('@xterm/headless'); - const term = new Terminal({ cols, rows, scrollback, allowProposedApi: true }); - await new Promise((resolve) => term.write(new Uint8Array(capture), resolve)); - const lines: string[] = []; - // Both buffers matter: a full-screen TUI lives in the alternate buffer while - // the pre-alt-screen output (and anything printed after it exits) is in the - // normal one. - for (const buf of [term.buffer.normal, term.buffer.active]) { - for (let y = 0; y < buf.length; y += 1) { - lines.push(buf.getLine(y)?.translateToString(true).trimEnd() ?? ''); +export async function recordEvents(brokerUrl: string, apiKey: string): Promise { + const { default: WebSocket } = await import('ws'); + const wsUrl = `${brokerUrl.replace(/^http/, 'ws')}/ws`; + const socket = new WebSocket(wsUrl, { headers: { 'X-API-Key': apiKey } }); + + const messages: RelayMessage[] = []; + const kinds: Record = {}; + + socket.on('message', (data: Buffer) => { + let frame: Record; + try { + frame = JSON.parse(data.toString('utf-8')) as Record; + } catch { + return; } - } - return lines.join('\n'); + const kind = typeof frame.kind === 'string' ? frame.kind : '(unknown)'; + kinds[kind] = (kinds[kind] ?? 0) + 1; + if (kind === 'relay_inbound') { + messages.push({ + from: String(frame.from ?? ''), + target: String(frame.target ?? ''), + body: String(frame.body ?? ''), + }); + } + }); + socket.on('error', () => {}); + + await new Promise((resolve, reject) => { + socket.once('open', () => resolve()); + socket.once('error', reject); + }); + + return { + messages: () => [...messages], + kinds: () => ({ ...kinds }), + stop: () => socket.close(), + }; } diff --git a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts index 22d872253..8bca6f3a5 100644 --- a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts +++ b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts @@ -32,13 +32,14 @@ import { attachInPty, cleanEnv, preflight, + recordEvents, renderScreen, - renderTranscript, startBroker, startEngine, waitFor, type BrokerHandle, type EngineHandle, + type EventRecorder, type PtyClient, } from './harness.js'; @@ -130,6 +131,7 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay let tmpRoot: string; let engine: EngineHandle; let broker: BrokerHandle; + let events: EventRecorder; const clients = new Map(); beforeAll(async () => { @@ -142,6 +144,7 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay engine = await startEngine(check.engineServe!, tmpRoot); broker = await startBroker(projectDir, engine.baseUrl, home); + events = await recordEvents(broker.url, broker.apiKey); for (const [name, task] of Object.entries(TASKS)) { await broker.spawnAgent(name, task); @@ -165,26 +168,23 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay // The game is three LLM agents taking turns over the network; give it room. // - // Wait on DONE_TOKEN reaching a PLAYER. The token appears only in the - // Gamemaster's prompt, so seeing it in a player's pane proves a real - // end-of-game message was delivered. Waiting on a phrase like 'GAME OVER' - // instead matches the injected task prompt itself and fires immediately. + // Wait for DONE_TOKEN to be DELIVERED to a player. The token is named only + // in the Gamemaster's prompt, so a delivery carrying it is proof the game + // really ended rather than the prompt echoing. (Waiting on a natural + // phrase like 'GAME OVER' matches the injected prompt itself and fires + // instantly; waiting on a rendered pane misses it once the alt-screen TUI + // repaints, since the alternate buffer keeps no scrollback.) await waitFor( - async () => { - for (const player of ['PlayerA', 'PlayerB'] as const) { - if ((await transcript(player)).includes(DONE_TOKEN)) return true; - } - return false; - }, + () => + events + .messages() + .some((m) => m.body.includes(DONE_TOKEN) && m.target !== 'Gamemaster'), { timeoutMs: 8 * 60_000, label: 'the game to finish', intervalMs: 5_000 } ); }, 10 * 60_000); - /** What the pane has shown so far, scrollback included. */ - const transcript = (name: string): Promise => - renderTranscript(clients.get(name)!.read(), COLS, ROWS); - afterAll(() => { + events?.stop(); for (const client of clients.values()) client.stop(); broker?.stop(); engine?.stop(); @@ -201,24 +201,32 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay } }); - it('carries moves between agents over the relay protocol', async () => { - const gm = await transcript('Gamemaster'); - // Both players' moves reached the Gamemaster as relay messages, and the - // Gamemaster saw them as relay traffic (not as its own typing). - expect(gm).toContain('Relay message from PlayerA'); - expect(gm).toContain('Relay message from PlayerB'); - expect(gm).toMatch(/MOVE\s*\d/); + it('carries moves between agents over the relay protocol', () => { + const delivered = events.messages(); + const between = (from: string, target: string) => + delivered.filter((m) => m.from === from && m.target === target); + // Both directions, both players — a game where only the Gamemaster ever + // spoke would still reach a "result", so assert the replies too. for (const player of ['PlayerA', 'PlayerB'] as const) { - expect(await transcript(player), `${player} never received a relay message`).toContain( - 'Relay message from Gamemaster' - ); + expect(between('Gamemaster', player).length, `Gamemaster never messaged ${player}`).toBeGreaterThan(0); + expect(between(player, 'Gamemaster').length, `${player} never replied`).toBeGreaterThan(0); } + + // The replies are moves, not chatter. + const moves = delivered.filter((m) => m.target === 'Gamemaster' && /MOVE\s*\d/i.test(m.body)); + expect(moves.length, `no MOVE replies in ${JSON.stringify(delivered)}`).toBeGreaterThanOrEqual(3); + + // And the panes carried live PTY output while it happened. + expect(events.kinds().worker_stream ?? 0).toBeGreaterThan(0); }); - it('reaches a terminal result and tells both players', async () => { + it('reaches a terminal result and tells both players', () => { for (const player of ['PlayerA', 'PlayerB'] as const) { - expect(await transcript(player), `${player} was never told the game ended`).toContain(DONE_TOKEN); + const finals = events + .messages() + .filter((m) => m.target === player && m.body.includes(DONE_TOKEN)); + expect(finals.length, `${player} was never told the game ended`).toBeGreaterThan(0); } }); From fede40499c885e061ff5b9ec9659b8533af94074 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 18:52:39 +0000 Subject: [PATCH 3/8] docs: drop an unverified issue reference from the status-line comment Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- packages/cli/src/cli/lib/attach.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/cli/lib/attach.ts b/packages/cli/src/cli/lib/attach.ts index 65ef98507..b490c58e0 100644 --- a/packages/cli/src/cli/lib/attach.ts +++ b/packages/cli/src/cli/lib/attach.ts @@ -387,8 +387,9 @@ export const DEFAULT_STATUS_LINE_COLS = 80; * then scrolls again — promoting the previous status line into the scrollback * as content and eating one row of the agent's output each time. A narrow * pane therefore turns a single status line into a growing stack of them with - * the agent's TUI shredded behind it (#1360 follow-up; reproduced at 66 - * columns, where an 87-column `drive` label cost six rows of agent output). + * the agent's TUI shredded behind it. The `drive` label is 87 columns, so this + * fired on a standard 80-column terminal too; at 66 columns six repaints cost + * six rows of agent output. * * Truncation keeps the paint inside one row, so the wrap — and the scroll * cascade it triggers — can never happen. The tail is the part that carries From bdb02a6645e35b2e2f0e6e01f632eb03b8871732 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 20:08:10 +0000 Subject: [PATCH 4/8] fix(broker): reap agents whose harness exited but whose wrapper lives on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `node agent list` reported a dead agent as `working` forever, with its last-activity frozen at the moment it died. The broker judged worker liveness solely by `handle.child` — the `agent-relay-broker pty …` wrapper. That wrapper can outlive the harness it hosts: its stdin reader blocks on a pipe the broker never closes, so a wrapper whose harness exited sits in `futex_do_wait` indefinitely, holding no pts fds and no children. `reap_exited` never fired, and the PTY runtime never sends `worker_exited` (only the headless and app-server runtimes do), so nothing else noticed either. Measured: still listed 240s after the harness died, and it never cleared — the wrapper had to be killed by hand. Liveness is now judged by the harness: - a reported harness pid that is gone (`kill(pid, 0)` → ESRCH) reaps the worker - a worker that never reports `worker_ready` within 90s is treated as failed-to-start Both paths kill the orphaned wrapper and flow through the existing exit handling, so lifecycle events, crash insights, and pending-delivery cleanup all still run. The 90s deadline only applies while no harness pid is known: a live pid is proof of life and is never reaped for missing readiness. It clears the PTY runtime's own 25s readiness fallback (`pty_worker::STARTUP_READY_TIMEOUT`) more than threefold — reaping a healthy-but-slow agent would be far worse than listing a dead one a little longer. Restarts are unaffected: a restart builds a fresh handle, so a stale pid is never probed against a live worker. Verified end to end — startup death: never cleared → 92s; harness killed mid-life: never cleared → 2s; orphaned wrapper cleaned up in both. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- CHANGELOG.md | 1 + crates/broker/src/runtime/tests.rs | 3 + crates/broker/src/runtime/worker_events.rs | 4 + crates/broker/src/worker.rs | 197 +++++++++++++++++++++ 4 files changed, 205 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 587201c17..fe12c1def 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Opting out of telemetry (`AGENT_RELAY_TELEMETRY_DISABLED` or `DO_NOT_TRACK`) now keeps your cloud identity out of child process environments, including identity an ancestor process or your shell had already exported. The identity env vars — one of which carries your email — previously reached every spawned process, including third-party harness CLIs, even when opted out. - Identity forwarding to the Relaycast gateway is no longer gated on the local process carrying a PostHog key. An npm-installed CLI bakes no key, so it previously forwarded no identity at all and every hosted event fell back to being keyed on the workspace. Forwarding now follows the telemetry preference alone. - `node agent attach --mode drive|passthrough` truncates its status line to the terminal width, preventing repeated repaints from scrolling away agent output. +- `node agent list` stops reporting an exited or never-ready harness as `working` and cleans up its orphaned wrapper process. - `node agent attach --mode view` now exits on the first Ctrl-C instead of waiting for a WebSocket close handshake. - The broker now sends its anonymous telemetry id (`X-Agent-Relay-Distinct-Id`) and origin actor with its Relaycast requests, so hosted usage can be attributed to an install instead of only to a workspace. The id header is omitted when telemetry is opted out; requests and origin actor are unaffected. - The broker now reads its telemetry preference and machine-id files from `AGENT_RELAY_DATA_DIR` when set, matching the CLI. It previously only read `~/.agentworkforce/relay/telemetry.json`, so an opt-out written by `agent-relay telemetry disable` under a configured data directory was ignored. diff --git a/crates/broker/src/runtime/tests.rs b/crates/broker/src/runtime/tests.rs index 56ffdb277..126640603 100644 --- a/crates/broker/src/runtime/tests.rs +++ b/crates/broker/src/runtime/tests.rs @@ -107,6 +107,9 @@ async fn make_worker_registry_with_worker(name: &str) -> WorkerRegistry { stdin, harness_pid: None, spawned_at: Instant::now(), + // Ready, so the orphan sweep's readiness deadline never applies to + // these fixtures. + ready_at: Some(Instant::now()), last_activity_at: Instant::now(), context_budget_pct: None, state: AgentWorkState::Working, diff --git a/crates/broker/src/runtime/worker_events.rs b/crates/broker/src/runtime/worker_events.rs index 5603e2bf8..ae6b76b5b 100644 --- a/crates/broker/src/runtime/worker_events.rs +++ b/crates/broker/src/runtime/worker_events.rs @@ -848,6 +848,10 @@ impl BrokerRuntime { if let Some(pid) = payload_pid { h.harness_pid = Some(pid); } + // Records that the harness actually came up, so + // `reap_exited` can tell a slow start from one that + // never happened. + h.ready_at.get_or_insert_with(Instant::now); ( h.spec.provider.clone(), h.spec.cli.clone(), diff --git a/crates/broker/src/worker.rs b/crates/broker/src/worker.rs index 5abcbbbb9..667f41718 100644 --- a/crates/broker/src/worker.rs +++ b/crates/broker/src/worker.rs @@ -42,6 +42,82 @@ const APP_SERVER_AUTH_ENV_KEYS: [&str; 4] = [ const DEFAULT_RELEASE_GRACE: Duration = Duration::from_secs(2); const APP_SERVER_RELEASE_GRACE: Duration = Duration::from_secs(35); +/// How long a worker may go without reporting `worker_ready` before the broker +/// treats its harness as failed-to-start. +/// +/// The worker process emits `worker_ready` itself — the PTY runtime even has a +/// 25s fallback that fires when readiness detection times out +/// (`pty_worker::STARTUP_READY_TIMEOUT`). So silence past this deadline does not +/// mean "slow": it means the worker died, or wedged, before it could report. +/// The margin over that 25s fallback is deliberately generous — reaping a +/// healthy-but-slow agent is far worse than listing a dead one a little longer. +const WORKER_READY_DEADLINE: Duration = Duration::from_secs(90); + +/// Why a worker was reaped despite its wrapper process still being alive. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum OrphanedWorker { + /// The harness pid the worker reported is gone. + HarnessExited, + /// `worker_ready` never arrived within {@link WORKER_READY_DEADLINE}. + NeverReady, +} + +impl OrphanedWorker { + fn reason(self) -> &'static str { + match self { + OrphanedWorker::HarnessExited => "harness_exited", + OrphanedWorker::NeverReady => "harness_never_ready", + } + } +} + +/// True when `pid` no longer names a live process. +/// +/// Safety: `kill(pid, 0)` is a POSIX-safe probe — it performs the permission +/// and existence checks without delivering a signal. `ESRCH` means the process +/// is gone; every other error (notably `EPERM`) means it exists. +#[cfg(unix)] +fn pid_is_gone(pid: u32) -> bool { + let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; + ret == -1 + && std::io::Error::last_os_error() + .raw_os_error() + .unwrap_or(0) + == libc::ESRCH +} + +#[cfg(not(unix))] +fn pid_is_gone(_pid: u32) -> bool { + false +} + +/// Decide whether a worker is dead even though its wrapper process is alive. +/// +/// The wrapper (`agent-relay-broker pty …`) can outlive the harness it hosts: +/// its stdin reader blocks on a pipe the broker never closes, so a wrapper whose +/// harness exited at startup can sit in `futex_do_wait` indefinitely. Reaping on +/// the wrapper alone therefore leaves the agent listed as `working` forever, with +/// `last_activity` frozen at the moment it died. Judge liveness by the harness. +pub(crate) fn orphaned_worker( + harness_pid: Option, + ready_at: Option, + spawned_at: Instant, + now: Instant, +) -> Option { + if let Some(pid) = harness_pid { + if pid_is_gone(pid) { + return Some(OrphanedWorker::HarnessExited); + } + // A live harness pid is proof of life; never apply the readiness + // deadline to one that is plainly running. + return None; + } + if ready_at.is_none() && now.saturating_duration_since(spawned_at) > WORKER_READY_DEADLINE { + return Some(OrphanedWorker::NeverReady); + } + None +} + // Working/idle activity inference from PTY output comes from the // harness-agnostic `relay-pty` crate. pub(crate) use relay_pty::detection; @@ -55,6 +131,9 @@ pub(crate) struct WorkerHandle { pub(crate) stdin: ChildStdin, pub(crate) harness_pid: Option, pub(crate) spawned_at: Instant, + /// When the worker reported `worker_ready`. `None` means the harness has + /// never come up — see `WORKER_READY_DEADLINE` in `reap_exited`. + pub(crate) ready_at: Option, pub(crate) last_activity_at: Instant, pub(crate) context_budget_pct: Option, pub(crate) state: AgentWorkState, @@ -867,6 +946,7 @@ impl WorkerRegistry { stdin, harness_pid: initial_harness_pid, spawned_at: Instant::now(), + ready_at: None, last_activity_at: Instant::now(), context_budget_pct: None, state: AgentWorkState::Working, @@ -1048,6 +1128,43 @@ impl WorkerRegistry { } else { (None, false) }; + // The wrapper can outlive its harness. When it does, judge the agent + // by the harness and tear the orphaned wrapper down — otherwise the + // agent is listed as `working` forever. + let orphaned = if status.is_none() && !gone_via_kill0 { + self.workers.get(&name).and_then(|handle| { + orphaned_worker( + handle.harness_pid, + handle.ready_at, + handle.spawned_at, + Instant::now(), + ) + }) + } else { + None + }; + if let Some(orphan) = orphaned { + let reason = self + .workers + .get(&name) + .and_then(|handle| handle.exit_reason.clone()) + .or_else(|| Some(orphan.reason().to_string())); + if let Some(handle) = self.workers.get_mut(&name) { + tracing::warn!( + worker = %name, + wrapper_pid = ?handle.child.id(), + harness_pid = ?handle.harness_pid, + reason = orphan.reason(), + "reap_exited: harness gone but wrapper alive — killing orphaned wrapper" + ); + // Best effort: the wrapper is unreachable by definition here. + let _ = handle.child.start_kill(); + } + self.workers.remove(&name); + self.initial_tasks.remove(&name); + exited.push((name, None, None, reason)); + continue; + } if let Some(status) = status { let code = status.code(); #[cfg(unix)] @@ -1898,6 +2015,86 @@ mod tests { assert!(reg.list(&HashMap::new()).is_empty()); } + // The wrapper process can outlive the harness it hosts, so reaping on the + // wrapper alone leaves a dead agent listed as `working` forever. + mod orphaned_worker { + use super::*; + + /// A pid that cannot be live. `kill(0)` on pid 0 addresses the caller's + /// own process group, so use an unassigned high pid instead. + fn dead_pid() -> u32 { + // Above the default pid_max; never allocated. + 0x7FFF_FFFF + } + + fn live_pid() -> u32 { + std::process::id() + } + + #[test] + fn reports_harness_exited_when_the_reported_pid_is_gone() { + let now = Instant::now(); + assert_eq!( + orphaned_worker(Some(dead_pid()), Some(now), now, now), + Some(OrphanedWorker::HarnessExited) + ); + } + + #[test] + fn leaves_a_worker_with_a_live_harness_alone() { + let now = Instant::now(); + assert_eq!(orphaned_worker(Some(live_pid()), Some(now), now, now), None); + } + + // A live harness pid is proof of life even if `worker_ready` was missed, + // so the readiness deadline must not apply to it. + #[test] + fn a_live_harness_is_never_reaped_for_missing_readiness() { + let spawned = Instant::now(); + let now = spawned + WORKER_READY_DEADLINE + Duration::from_secs(60); + assert_eq!(orphaned_worker(Some(live_pid()), None, spawned, now), None); + } + + #[test] + fn reports_never_ready_only_after_the_deadline() { + let spawned = Instant::now(); + // Still inside the window — a slow harness must be left to boot. + assert_eq!( + orphaned_worker(None, None, spawned, spawned + Duration::from_secs(30)), + None + ); + assert_eq!( + orphaned_worker(None, None, spawned, spawned + WORKER_READY_DEADLINE), + None + ); + assert_eq!( + orphaned_worker( + None, + None, + spawned, + spawned + WORKER_READY_DEADLINE + Duration::from_secs(1) + ), + Some(OrphanedWorker::NeverReady) + ); + } + + // A worker that reported ready and has no pid to probe (non-PTY + // runtimes) must never be reaped by the deadline. + #[test] + fn a_ready_worker_without_a_pid_is_never_reaped() { + let spawned = Instant::now(); + let now = spawned + WORKER_READY_DEADLINE * 10; + assert_eq!(orphaned_worker(None, Some(spawned), spawned, now), None); + } + + #[test] + fn the_deadline_clears_the_pty_runtime_startup_fallback() { + // `pty_worker::STARTUP_READY_TIMEOUT` is 25s; the broker must wait + // comfortably longer than the worker's own fallback. + assert!(WORKER_READY_DEADLINE > Duration::from_secs(25) * 3); + } + } + #[test] fn has_worker_returns_false_for_unknown() { let reg = make_registry(vec![]); From e3a5967c0d114c806aa0d3c2d23a40e7e2f912e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 20:11:44 +0000 Subject: [PATCH 5/8] style: apply rustfmt to the orphan-worker probe Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- crates/broker/src/worker.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/broker/src/worker.rs b/crates/broker/src/worker.rs index 667f41718..7312bfad2 100644 --- a/crates/broker/src/worker.rs +++ b/crates/broker/src/worker.rs @@ -79,11 +79,7 @@ impl OrphanedWorker { #[cfg(unix)] fn pid_is_gone(pid: u32) -> bool { let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; - ret == -1 - && std::io::Error::last_os_error() - .raw_os_error() - .unwrap_or(0) - == libc::ESRCH + ret == -1 && std::io::Error::last_os_error().raw_os_error().unwrap_or(0) == libc::ESRCH } #[cfg(not(unix))] From 4e9c360510c9a6cb3f956af928fa1df8b5440e6f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 25 Jul 2026 20:13:27 +0000 Subject: [PATCH 6/8] style: auto-format with Prettier --- packages/cli/src/cli/lib/attach-drive.test.ts | 4 +++- .../cli/src/cli/lib/attach-passthrough.ts | 3 +-- tests/e2e/tic-tac-toe/README.md | 16 +++++++-------- tests/e2e/tic-tac-toe/harness.ts | 12 +++++------ tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts | 20 ++++++++++--------- 5 files changed, 29 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/cli/lib/attach-drive.test.ts b/packages/cli/src/cli/lib/attach-drive.test.ts index d95150edb..fa3408fe7 100644 --- a/packages/cli/src/cli/lib/attach-drive.test.ts +++ b/packages/cli/src/cli/lib/attach-drive.test.ts @@ -637,7 +637,9 @@ describe('renderStatusLine', () => { cols: 120, }); const text = stripStatusLineAnsi(out); - expect(text).toBe('[drive Gamemaster | delivery=manual_flush | pending=0 | Ctrl+] deliver | Ctrl+C detach]'); + expect(text).toBe( + '[drive Gamemaster | delivery=manual_flush | pending=0 | Ctrl+] deliver | Ctrl+C detach]' + ); expect(text).not.toContain('…'); }); diff --git a/packages/cli/src/cli/lib/attach-passthrough.ts b/packages/cli/src/cli/lib/attach-passthrough.ts index 4364ce80c..dfc1219ea 100644 --- a/packages/cli/src/cli/lib/attach-passthrough.ts +++ b/packages/cli/src/cli/lib/attach-passthrough.ts @@ -419,8 +419,7 @@ export async function runPassthroughSession( // Boundary-held + coalesced status painter (skips non-TTY stdout). const statusController = new StatusLineController({ - render: () => - renderStatusLine({ name, mode: 'auto_inject', rows: terminalRows, cols: terminalCols }), + render: () => renderStatusLine({ name, mode: 'auto_inject', rows: terminalRows, cols: terminalCols }), write: deps.writeChunk, enabled: statusLineEnabled, coalesceMs: deps.statusRepaintCoalesceMs ?? 40, diff --git a/tests/e2e/tic-tac-toe/README.md b/tests/e2e/tic-tac-toe/README.md index 8a58c1cce..1c0517a50 100644 --- a/tests/e2e/tic-tac-toe/README.md +++ b/tests/e2e/tic-tac-toe/README.md @@ -14,7 +14,7 @@ emulator (`@xterm/headless`) and asserts the agent's screen survives. This is the regression guard for the bug that made a `drive` pane unreadable: > The status line is painted on the bottom row inside an `ESC 7` / `ESC 8` -> (DECSC/DECRC) pair. That is only safe while the label *fits*. A label wider +> (DECSC/DECRC) pair. That is only safe while the label _fits_. A label wider > than the pane wraps past the last row, which scrolls the screen — and because > the label sits ON the bottom row, the next repaint scrolls again. Old status > lines get promoted into the scrollback as content and the agent's TUI loses a @@ -38,11 +38,11 @@ Asserts: | Assertion | Guards | | --------------------------------------------------------- | ------------------------------------------------------------------------------ | -| view streams grow | a `view` pane frozen on its attach snapshot (live `worker_stream` never lands) | -| `relay_inbound` in both directions, for both players | agents actually using the relay protocol rather than talking to themselves | -| ≥3 delivered replies matching `MOVE ` | a game that "ended" without anyone actually playing | -| the end-of-game token is delivered to both players | the round trip completes, not just the first hop | -| every pane renders coherently, no row wider than the pane | wrap/scroll corruption in the live stream | +| view streams grow | a `view` pane frozen on its attach snapshot (live `worker_stream` never lands) | +| `relay_inbound` in both directions, for both players | agents actually using the relay protocol rather than talking to themselves | +| ≥3 delivered replies matching `MOVE ` | a game that "ended" without anyone actually playing | +| the end-of-game token is delivered to both players | the round trip completes, not just the first hop | +| every pane renders coherently, no row wider than the pane | wrap/scroll corruption in the live stream | **Where each assertion runs matters.** Protocol claims read the broker's `relay_inbound` event stream; visual claims replay the PTY capture through an @@ -82,7 +82,7 @@ next person doesn't rediscover them. - **A fresh engine DB per run is mandatory.** The broker enrolls its node under the project directory name. Re-enrolling an already-known name fails `node_name_conflict`, after which the engine has no delivery-ready provider - for that node and defers *every* message + for that node and defers _every_ message (`[delivery.route] provider not delivery-ready`). Relay messaging then looks silently broken — agents stay "working" in `agent list` and never receive a thing. @@ -109,5 +109,5 @@ next person doesn't rediscover them. - **Don't wait on a natural phrase.** The task prompt is injected into the agent's PTY, so waiting for "GAME OVER" to appear matches the prompt itself and fires instantly. The suite waits on `DONE_TOKEN`, which only the - Gamemaster's prompt names — seeing it *delivered to a player* is proof the + Gamemaster's prompt names — seeing it _delivered to a player_ is proof the game really ended. diff --git a/tests/e2e/tic-tac-toe/harness.ts b/tests/e2e/tic-tac-toe/harness.ts index 496d4f082..47942e58f 100644 --- a/tests/e2e/tic-tac-toe/harness.ts +++ b/tests/e2e/tic-tac-toe/harness.ts @@ -241,7 +241,11 @@ export async function startBroker( function runCli(args: string[], cwd: string, env: NodeJS.ProcessEnv): Promise { return new Promise((resolve, reject) => { - const child = spawn(process.execPath, [RELAY_CLI, ...args], { cwd, env, stdio: ['ignore', 'pipe', 'pipe'] }); + const child = spawn(process.execPath, [RELAY_CLI, ...args], { + cwd, + env, + stdio: ['ignore', 'pipe', 'pipe'], + }); let out = ''; child.stdout?.on('data', (d) => (out += String(d))); child.stderr?.on('data', (d) => (out += String(d))); @@ -317,11 +321,7 @@ export function attachInPty(opts: { * "it was painted six times and scrolled the agent's output away" — both are * the same bytes on the wire. Only an emulator sees what the human sees. */ -export async function renderScreen( - capture: Buffer, - cols: number, - rows: number -): Promise { +export async function renderScreen(capture: Buffer, cols: number, rows: number): Promise { const { Terminal } = await import('@xterm/headless'); const term = new Terminal({ cols, rows, allowProposedApi: true }); await new Promise((resolve) => term.write(new Uint8Array(capture), resolve)); diff --git a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts index 8bca6f3a5..29c87dc7a 100644 --- a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts +++ b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts @@ -103,7 +103,8 @@ describe('attach status line survives a narrow pane', () => { cols, rows: ROWS, repaints: 6, - render: () => renderPassthroughStatusLine({ name: 'Gamemaster', mode: 'auto_inject', rows: ROWS, cols }), + render: () => + renderPassthroughStatusLine({ name: 'Gamemaster', mode: 'auto_inject', rows: ROWS, cols }), }); expect(screen.filter((l) => l.includes('[passthrough')).length).toBe(1); @@ -112,7 +113,13 @@ describe('attach status line survives a narrow pane', () => { }); it('keeps the agent name and the detach hint readable even when truncated', () => { - const line = renderStatusLine({ name: 'Gamemaster', mode: 'manual_flush', pending: 2, rows: 24, cols: 66 }); + const line = renderStatusLine({ + name: 'Gamemaster', + mode: 'manual_flush', + pending: 2, + rows: 24, + cols: 66, + }); expect(line).toContain('[drive Gamemaster'); expect(line).toContain('Ctrl+C detach]'); }); @@ -175,10 +182,7 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay // instantly; waiting on a rendered pane misses it once the alt-screen TUI // repaints, since the alternate buffer keeps no scrollback.) await waitFor( - () => - events - .messages() - .some((m) => m.body.includes(DONE_TOKEN) && m.target !== 'Gamemaster'), + () => events.messages().some((m) => m.body.includes(DONE_TOKEN) && m.target !== 'Gamemaster'), { timeoutMs: 8 * 60_000, label: 'the game to finish', intervalMs: 5_000 } ); }, 10 * 60_000); @@ -223,9 +227,7 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay it('reaches a terminal result and tells both players', () => { for (const player of ['PlayerA', 'PlayerB'] as const) { - const finals = events - .messages() - .filter((m) => m.target === player && m.body.includes(DONE_TOKEN)); + const finals = events.messages().filter((m) => m.target === player && m.body.includes(DONE_TOKEN)); expect(finals.length, `${player} was never told the game ended`).toBeGreaterThan(0); } }); From d00e7094751c387373037f9f43ff37aa65544400 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 20:37:41 +0000 Subject: [PATCH 7/8] fix: address review feedback on the attach and orphan-reap changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - Clamp the status line by display columns, not UTF-16 code units. A CJK or emoji agent name is double-width, so a code-unit count passed labels that still wrapped — re-arming the exact scroll cascade the clamp exists to prevent. Slicing now walks whole code points, so a surrogate pair is never split. - Reap the SIGKILLed orphan wrapper instead of dropping its handle. Leaving it to tokio's best-effort background reaper accumulates zombies across repeated failed agent starts; the wait is bounded so a wrapper wedged in uninterruptible sleep cannot stall the maintenance tick, which also drives delivery retries. - `pty-run.py` lost the child's exit status on the EOF/EIO path, so a crashed attach client reported a clean run and the cleanup SIGTERMed a dead pid. Verified: 0, 7, 137 (SIGKILL) and 127 (missing binary) now propagate. - The E2E teardown deleted the project tree without awaiting `node down`. If deletion won the race, `down` lost the connection file it identifies the daemon by and left the broker — and its Claude workers — running past the suite. - The README's live-run command had a comment after a line continuation, so the backslash escaped a space rather than the newline: `npm run test:e2e` ran without `RELAY_TTT_LIVE`, silently skipping the live half. Hardening (CodeQL, all in the new harness): - Count event kinds in a Map. `kind` is arbitrary text off the wire, so `kinds[kind] = …` on a plain object let a `"kind":"__proto__"` frame reach `Object.prototype`. - Validate the broker URL read from `connection.json` and rebuild it from its parts, pinning it to a loopback origin — stale or tampered state can no longer aim test traffic (with an API key attached) at an arbitrary host. Also: static `spawnSync` import rather than `require` in an ESM module; `recordEvents` now rejects if the socket closes before it opens instead of hanging until the hook timeout; the live-stream assertion grows from a per-pane snapshot baseline rather than a guessed byte floor; `pid_is_gone` and `readScreen` de-duplicate probes and screen reads; and the changelog entries are cut to one impact-first line each per AGENTS.md. Verified: cargo test 1073 passed, clippy/fmt clean, prettier clean, eslint at the 41-warning baseline with none added, live tic-tac-toe E2E 10/10. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- CHANGELOG.md | 4 +- crates/broker/src/worker.rs | 66 +++++----- packages/cli/src/cli/lib/attach.test.ts | 55 +++++++++ packages/cli/src/cli/lib/attach.ts | 80 ++++++++++-- tests/e2e/tic-tac-toe/README.md | 3 +- tests/e2e/tic-tac-toe/harness.ts | 114 ++++++++++++++---- tests/e2e/tic-tac-toe/pty-run.py | 24 ++++ tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts | 42 +++++-- 8 files changed, 311 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe12c1def..0daac1d24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,8 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The published CLI now actually reports telemetry. The npm package is plain `tsc` output with no key injection step, and the bun standalone's `--define` targeted a literal `process.env.AGENT_RELAY_POSTHOG_KEY` that the code never read (it used a computed `process.env[name]` lookup), so **both** installable artifacts shipped with telemetry silently disabled — every `cli_command_run`, `workflow_run`, `cloud_auth`, `agent_relay_tool_call`, `setup_init`, `swarm_run`, and `bridge_spawn` event was dropped. Only the Rust broker was reporting. - Opting out of telemetry (`AGENT_RELAY_TELEMETRY_DISABLED` or `DO_NOT_TRACK`) now keeps your cloud identity out of child process environments, including identity an ancestor process or your shell had already exported. The identity env vars — one of which carries your email — previously reached every spawned process, including third-party harness CLIs, even when opted out. - Identity forwarding to the Relaycast gateway is no longer gated on the local process carrying a PostHog key. An npm-installed CLI bakes no key, so it previously forwarded no identity at all and every hosted event fell back to being keyed on the workspace. Forwarding now follows the telemetry preference alone. -- `node agent attach --mode drive|passthrough` truncates its status line to the terminal width, preventing repeated repaints from scrolling away agent output. -- `node agent list` stops reporting an exited or never-ready harness as `working` and cleans up its orphaned wrapper process. +- `node agent list` no longer reports an exited agent as `working` indefinitely; the broker now reaps a worker whose harness has gone away or never started. +- `node agent attach --mode drive|passthrough` truncates its status line to the terminal width, so a narrow pane no longer stacks status lines over the agent's output. - `node agent attach --mode view` now exits on the first Ctrl-C instead of waiting for a WebSocket close handshake. - The broker now sends its anonymous telemetry id (`X-Agent-Relay-Distinct-Id`) and origin actor with its Relaycast requests, so hosted usage can be attributed to an install instead of only to a workspace. The id header is omitted when telemetry is opted out; requests and origin actor are unaffected. - The broker now reads its telemetry preference and machine-id files from `AGENT_RELAY_DATA_DIR` when set, matching the CLI. It previously only read `~/.agentworkforce/relay/telemetry.json`, so an opt-out written by `agent-relay telemetry disable` under a configured data directory was ignored. diff --git a/crates/broker/src/worker.rs b/crates/broker/src/worker.rs index 7312bfad2..798f20c2b 100644 --- a/crates/broker/src/worker.rs +++ b/crates/broker/src/worker.rs @@ -53,12 +53,17 @@ const APP_SERVER_RELEASE_GRACE: Duration = Duration::from_secs(35); /// healthy-but-slow agent is far worse than listing a dead one a little longer. const WORKER_READY_DEADLINE: Duration = Duration::from_secs(90); +/// How long to wait for a SIGKILLed orphan wrapper to be reaped before giving +/// up. Bounded so a wrapper stuck in uninterruptible sleep cannot stall the +/// maintenance tick, which also drives delivery retries. +const ORPHAN_REAP_TIMEOUT: Duration = Duration::from_secs(2); + /// Why a worker was reaped despite its wrapper process still being alive. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum OrphanedWorker { /// The harness pid the worker reported is gone. HarnessExited, - /// `worker_ready` never arrived within {@link WORKER_READY_DEADLINE}. + /// `worker_ready` never arrived within [`WORKER_READY_DEADLINE`]. NeverReady, } @@ -307,16 +312,7 @@ impl WorkerRegistry { #[cfg(unix)] { match handle.child.id() { - // Safety: kill(pid, 0) is a POSIX-safe probe that checks process - // existence without sending a signal. ESRCH => the process is gone. - Some(pid) => { - let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; - if ret == -1 { - std::io::Error::last_os_error().raw_os_error().unwrap_or(0) != libc::ESRCH - } else { - true - } - } + Some(pid) => !pid_is_gone(pid), // `id()` returns None once the child has been waited/reaped. None => false, } @@ -1079,24 +1075,13 @@ impl WorkerRegistry { #[cfg(unix)] { if let Some(pid) = handle.child.id() { - // Safety: kill(pid, 0) is a POSIX-safe probe that checks - // process existence without sending a signal. ESRCH means - // the process no longer exists. - let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; - if ret == -1 { - let errno = std::io::Error::last_os_error() - .raw_os_error() - .unwrap_or(0); - if errno == libc::ESRCH { - tracing::info!( - worker = %name, - pid = pid, - "reap_exited: kill(0) says ESRCH — process gone" - ); - (None, true) - } else { - (None, false) - } + if pid_is_gone(pid) { + tracing::info!( + worker = %name, + pid = pid, + "reap_exited: kill(0) says ESRCH — process gone" + ); + (None, true) } else { (None, false) } @@ -1153,8 +1138,27 @@ impl WorkerRegistry { reason = orphan.reason(), "reap_exited: harness gone but wrapper alive — killing orphaned wrapper" ); - // Best effort: the wrapper is unreachable by definition here. - let _ = handle.child.start_kill(); + // SIGKILL *and* reap. Dropping the `Child` without waiting + // leaves the wrapper to tokio's best-effort background + // reaper, so a run of failed agent starts accumulates + // zombies. SIGKILL cannot be caught, so this returns + // promptly — but the deadline keeps a wrapper wedged in + // uninterruptible sleep from stalling the maintenance tick, + // which also drives delivery retries. + if let Err(error) = handle.child.start_kill() { + tracing::warn!(worker = %name, %error, "failed to signal orphaned wrapper"); + } + match timeout(ORPHAN_REAP_TIMEOUT, handle.child.wait()).await { + Ok(Ok(_)) => {} + Ok(Err(error)) => { + tracing::warn!(worker = %name, %error, "orphaned wrapper wait failed") + } + Err(_) => tracing::warn!( + worker = %name, + timeout_ms = ORPHAN_REAP_TIMEOUT.as_millis(), + "orphaned wrapper did not exit before the reap deadline" + ), + } } self.workers.remove(&name); self.initial_tasks.remove(&name); diff --git a/packages/cli/src/cli/lib/attach.test.ts b/packages/cli/src/cli/lib/attach.test.ts index 16e4ee39e..cf7e82ff2 100644 --- a/packages/cli/src/cli/lib/attach.test.ts +++ b/packages/cli/src/cli/lib/attach.test.ts @@ -803,6 +803,41 @@ describe('pickInitialTerminalCols', () => { }); }); +/** + * Mirror of the renderer's column accounting, for assertions only. + * + * Deliberately a separate implementation rather than importing the production + * one — a width assertion that reuses the code under test proves nothing. + */ +const ZERO_WIDTH: ReadonlyArray = [ + [0x0300, 0x036f], + [0x200d, 0x200d], + [0xfe00, 0xfe0f], +]; +const DOUBLE_WIDTH: ReadonlyArray = [ + [0x1100, 0x115f], + [0x2e80, 0xa4cf], + [0xac00, 0xd7a3], + [0xf900, 0xfaff], + [0xfe30, 0xfe6f], + [0xff00, 0xff60], + [0xffe0, 0xffe6], + [0x1f300, 0x1faff], + [0x20000, 0x3fffd], +]; + +function columnsOf(text: string): number { + const hit = (code: number, ranges: ReadonlyArray) => + ranges.some(([lo, hi]) => code >= lo && code <= hi); + let total = 0; + for (const char of text) { + const code = char.codePointAt(0) ?? 0; + if (hit(code, ZERO_WIDTH)) continue; + total += hit(code, DOUBLE_WIDTH) ? 2 : 1; + } + return total; +} + describe('clampStatusLineText', () => { const label = '[drive Gamemaster | delivery=manual_flush | pending=0 | Ctrl+] deliver | Ctrl+C detach]'; @@ -829,6 +864,26 @@ describe('clampStatusLineText', () => { expect(clampStatusLineText(label, 0).length).toBeLessThanOrEqual(DEFAULT_STATUS_LINE_COLS); }); + // A code-unit count would pass these while the row still overflows, which + // re-arms the wrap/scroll cascade the clamp exists to prevent. + it('measures wide glyphs as two columns', () => { + const wide = '[drive 名前ですよろしく | delivery=manual_flush | Ctrl+C detach]'; + for (const cols of [20, 40, 66]) { + const out = clampStatusLineText(wide, cols); + expect(columnsOf(out), `overflowed at ${cols}: ${JSON.stringify(out)}`).toBeLessThanOrEqual(cols); + } + }); + + it('never splits a surrogate pair', () => { + const emoji = `[drive ${'\u{1F680}'.repeat(20)} | Ctrl+C detach]`; + for (const cols of [10, 25, 50]) { + const out = clampStatusLineText(emoji, cols); + expect(out).not.toMatch(/[\uD800-\uDBFF](?![\uDC00-\uDFFF])/); + expect(out).not.toMatch(/(? { expect(clampStatusLineText(label, 1)).toBe('['); expect(clampStatusLineText(label, 0).length).toBeLessThanOrEqual(DEFAULT_STATUS_LINE_COLS); diff --git a/packages/cli/src/cli/lib/attach.ts b/packages/cli/src/cli/lib/attach.ts index b490c58e0..708573539 100644 --- a/packages/cli/src/cli/lib/attach.ts +++ b/packages/cli/src/cli/lib/attach.ts @@ -395,18 +395,84 @@ export const DEFAULT_STATUS_LINE_COLS = 80; * cascade it triggers — can never happen. The tail is the part that carries * the key hints, so an over-long label is trimmed from the *middle*: the verb * and agent name stay readable and the `Ctrl+…` hints survive. + * + * Measured in display columns, not UTF-16 code units: an agent name carrying + * CJK or emoji is double-width, so a code-unit count would pass a label that + * still wraps — re-arming the exact cascade this prevents. Slicing is done on + * whole code points so a surrogate pair is never split in half. */ export function clampStatusLineText(text: string, cols: number | undefined): string { const width = typeof cols === 'number' && cols > 0 ? Math.floor(cols) : DEFAULT_STATUS_LINE_COLS; - if (text.length <= width) return text; + if (displayWidth(text) <= width) return text; // Too narrow to say anything useful — a bare head is still better than a // wrap, and `…` alone would be meaningless. - if (width <= 1) return text.slice(0, Math.max(width, 0)); - const ellipsis = '…'; - const keep = width - ellipsis.length; - const tail = Math.floor(keep / 2); - const head = keep - tail; - return `${text.slice(0, head)}${ellipsis}${tail > 0 ? text.slice(text.length - tail) : ''}`; + if (width <= 1) return width === 1 ? takeColumns(text, 1).text : ''; + const keep = width - 1; // the ellipsis occupies one column + const tailBudget = Math.floor(keep / 2); + const headBudget = keep - tailBudget; + const head = takeColumns(text, headBudget); + const tail = tailBudget > 0 ? takeColumns(text, tailBudget, 'end') : { text: '' }; + return `${head.text}…${tail.text}`; +} + +/** + * Column width of a string, counting East Asian Wide/Fullwidth characters and + * emoji as two columns and zero-width joiners/combining marks as none. This is + * the pragmatic subset a status line needs — it is not a full `wcwidth`. + */ +function displayWidth(text: string): number { + let total = 0; + for (const char of text) total += charWidth(char); + return total; +} + +/** Codepoints that occupy no column: combining marks, ZWJ, variation selectors. */ +const ZERO_WIDTH_RANGES: ReadonlyArray = [ + [0x0300, 0x036f], + [0x200d, 0x200d], + [0xfe00, 0xfe0f], +]; + +/** East Asian Wide / Fullwidth blocks, plus the emoji planes. */ +const DOUBLE_WIDTH_RANGES: ReadonlyArray = [ + [0x1100, 0x115f], + [0x2e80, 0xa4cf], + [0xac00, 0xd7a3], + [0xf900, 0xfaff], + [0xfe30, 0xfe6f], + [0xff00, 0xff60], + [0xffe0, 0xffe6], + [0x1f300, 0x1faff], + [0x20000, 0x3fffd], +]; + +function inRanges(code: number, ranges: ReadonlyArray): boolean { + return ranges.some(([lo, hi]) => code >= lo && code <= hi); +} + +function charWidth(char: string): number { + const code = char.codePointAt(0) ?? 0; + if (inRanges(code, ZERO_WIDTH_RANGES)) return 0; + return inRanges(code, DOUBLE_WIDTH_RANGES) ? 2 : 1; +} + +/** + * Take whole code points from `text` until `budget` columns are used, from the + * start or the `end`. Never splits a surrogate pair, and never overshoots the + * budget — a double-width character that would exceed it is dropped. + */ +function takeColumns(text: string, budget: number, from: 'start' | 'end' = 'start'): { text: string } { + const chars = Array.from(text); + const ordered = from === 'end' ? chars.slice().reverse() : chars; + const taken: string[] = []; + let used = 0; + for (const char of ordered) { + const w = charWidth(char); + if (used + w > budget) break; + used += w; + taken.push(char); + } + return { text: (from === 'end' ? taken.reverse() : taken).join('') }; } /** diff --git a/tests/e2e/tic-tac-toe/README.md b/tests/e2e/tic-tac-toe/README.md index 1c0517a50..cab349079 100644 --- a/tests/e2e/tic-tac-toe/README.md +++ b/tests/e2e/tic-tac-toe/README.md @@ -63,8 +63,9 @@ emulator. Neither substitutes for the other: npm run build:core # relay CLI + packages cargo build --release --bin agent-relay-broker # broker +# RELAYCAST_ENGINE_DIR must point at a *built* relaycast checkout. RELAY_TTT_LIVE=1 \ -RELAYCAST_ENGINE_DIR=/path/to/relaycast \ # a built relaycast checkout +RELAYCAST_ENGINE_DIR=/path/to/relaycast \ npm run test:e2e -- tests/e2e/tic-tac-toe ``` diff --git a/tests/e2e/tic-tac-toe/harness.ts b/tests/e2e/tic-tac-toe/harness.ts index 47942e58f..cc76e7c2e 100644 --- a/tests/e2e/tic-tac-toe/harness.ts +++ b/tests/e2e/tic-tac-toe/harness.ts @@ -9,7 +9,7 @@ * agent's output) is invisible through a pipe. */ -import { spawn, type ChildProcess } from 'node:child_process'; +import { spawn, spawnSync, type ChildProcess } from 'node:child_process'; import { existsSync, readFileSync, statSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -77,8 +77,6 @@ function hasPython3(): boolean { } function spawnSyncQuiet(cmd: string, args: string[]): number { - // eslint-disable-next-line @typescript-eslint/no-var-requires -- sync probe only - const { spawnSync } = require('node:child_process') as typeof import('node:child_process'); const res = spawnSync(cmd, args, { stdio: 'ignore' }); return res.status ?? 1; } @@ -159,6 +157,35 @@ export async function startEngine(serveBin: string, tmpRoot: string): Promise; spawnAgent(name: string, task: string): Promise; - stop(): void; + /** Await this before deleting the project dir — see the implementation. */ + stop(): Promise; } /** @@ -200,40 +228,46 @@ export async function startBroker( timeoutMs: 30_000, label: 'broker connection.json', }); - const connection = JSON.parse(readFileSync(connectionPath, 'utf-8')) as { + const parsed = JSON.parse(readFileSync(connectionPath, 'utf-8')) as { url: string; api_key: string; }; + // Normalize before anything is fetched. `connection.json` is on-disk state, + // so its `url` is untrusted input to every request below; pinning it to a + // loopback origin means a stale or tampered file can only ever point the + // harness at a local broker, never at an arbitrary host. + const brokerUrl = requireLoopbackUrl(parsed.url, connectionPath); + const apiKey = String(parsed.api_key ?? ''); - const headers = { 'X-API-Key': connection.api_key, 'Content-Type': 'application/json' }; + const headers = { 'X-API-Key': apiKey, 'Content-Type': 'application/json' }; return { - url: connection.url, - apiKey: connection.api_key, + url: brokerUrl, + apiKey, send: (to, from, text) => - fetch(`${connection.url}/api/send`, { + fetch(`${brokerUrl}/api/send`, { method: 'POST', headers, body: JSON.stringify({ to, from, text }), }), async snapshot(name) { - const res = await fetch(`${connection.url}/api/spawned/${encodeURIComponent(name)}/snapshot`, { - headers: { 'X-API-Key': connection.api_key }, + const res = await fetch(`${brokerUrl}/api/spawned/${encodeURIComponent(name)}/snapshot`, { + headers: { 'X-API-Key': apiKey }, }); return (await res.json()) as { screen?: string; rows?: number; cols?: number }; }, async spawnAgent(name, task) { await runCli(['node', 'agent', 'spawn', 'claude', `--name=${name}`, '--task', task], projectDir, env); }, - stop() { + // Must be awaited before the caller deletes `projectDir`: `node down` + // identifies the daemon from `.agentworkforce/relay/connection.json`, so a + // teardown that removes the tree first leaves the broker — and every Claude + // worker under it — running past the end of the suite. + async stop() { try { - spawn(process.execPath, [RELAY_CLI, 'node', 'down', '--force'], { - cwd: projectDir, - env, - stdio: 'ignore', - }).unref(); + await runCli(['node', 'down', '--force'], projectDir, env); } catch { - // best effort + // Best effort: a broker that already died fails this, which is fine. } }, }; @@ -325,6 +359,21 @@ export async function renderScreen(capture: Buffer, cols: number, rows: number): const { Terminal } = await import('@xterm/headless'); const term = new Terminal({ cols, rows, allowProposedApi: true }); await new Promise((resolve) => term.write(new Uint8Array(capture), resolve)); + return readScreen(term, rows); +} + +/** The slice of `@xterm/headless`'s Terminal that {@link readScreen} needs. */ +interface ReadableTerminal { + buffer: { active: { getLine(y: number): { translateToString(trim: boolean): string } | undefined } }; +} + +/** + * Read a headless terminal's visible grid as one trimmed string per row. + * + * Shared so the capture-replay path and the synthetic status-line path cannot + * drift in how they read a screen. + */ +export function readScreen(term: ReadableTerminal, rows: number): string[] { const buf = term.buffer.active; const out: string[] = []; for (let y = 0; y < rows; y += 1) { @@ -363,11 +412,18 @@ export interface EventRecorder { */ export async function recordEvents(brokerUrl: string, apiKey: string): Promise { const { default: WebSocket } = await import('ws'); - const wsUrl = `${brokerUrl.replace(/^http/, 'ws')}/ws`; - const socket = new WebSocket(wsUrl, { headers: { 'X-API-Key': apiKey } }); + // Re-validate: `recordEvents` is exported, so it must not assume its caller + // already pinned the origin to loopback. + const origin = new URL(requireLoopbackUrl(brokerUrl, 'recordEvents')); + const socket = new WebSocket(`ws://${origin.hostname}:${origin.port}/ws`, { + headers: { 'X-API-Key': apiKey }, + }); const messages: RelayMessage[] = []; - const kinds: Record = {}; + // A Map, not an object: `kind` is an arbitrary string off the wire, and + // `kinds[kind] = …` on a plain object lets a frame with `"kind":"__proto__"` + // reach `Object.prototype`. + const kinds = new Map(); socket.on('message', (data: Buffer) => { let frame: Record; @@ -377,7 +433,7 @@ export async function recordEvents(brokerUrl: string, apiKey: string): Promise {}); await new Promise((resolve, reject) => { - socket.once('open', () => resolve()); + // A broker that accepts the TCP connection and then closes (an auth + // rejection arrives that way) would otherwise never settle this promise, + // hanging `beforeAll` until the hook timeout with no useful diagnostic. + const onClose = (code: number) => + reject(new Error(`broker closed the event socket before it opened (code ${code})`)); + socket.once('close', onClose); socket.once('error', reject); + socket.once('open', () => { + socket.off('close', onClose); + resolve(); + }); }); return { messages: () => [...messages], - kinds: () => ({ ...kinds }), + // `fromEntries` defines own properties, so a `__proto__` kind stays inert. + kinds: () => Object.fromEntries(kinds), stop: () => socket.close(), }; } diff --git a/tests/e2e/tic-tac-toe/pty-run.py b/tests/e2e/tic-tac-toe/pty-run.py index d37006fe4..7df58be8e 100644 --- a/tests/e2e/tic-tac-toe/pty-run.py +++ b/tests/e2e/tic-tac-toe/pty-run.py @@ -33,6 +33,24 @@ def set_winsize(fd: int, rows: int, cols: int) -> None: fcntl.ioctl(fd, termios.TIOCSWINSZ, struct.pack("HHHH", rows, cols, 0, 0)) +def reap(pid: int) -> "tuple[int, int]": + """Wait for `pid` and translate its status to a shell-style exit code. + + Returns `(exit_code, remaining_pid)`; `remaining_pid` is 0 once reaped, so + the caller's cleanup knows not to signal it again. + """ + try: + waited, status = os.waitpid(pid, 0) + except ChildProcessError: + return 0, 0 + if waited != pid: + return 0, pid + code = ( + os.WEXITSTATUS(status) if os.WIFEXITED(status) else 128 + os.WTERMSIG(status) + ) + return code, 0 + + def main() -> int: parser = argparse.ArgumentParser() parser.add_argument("--out", required=True) @@ -84,6 +102,11 @@ def main() -> int: raise data = b"" if not data: + # EOF/EIO means the child closed the slave side. Reap it + # here so its status is not lost — otherwise a crashed + # attach client reports a clean exit and `finally` SIGTERMs + # an already-dead pid. + exit_code, pid = reap(pid) break out.write(data) @@ -117,6 +140,7 @@ def main() -> int: ) pid = 0 break + finally: out.close() if pid: diff --git a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts index 29c87dc7a..770270e36 100644 --- a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts +++ b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts @@ -32,6 +32,7 @@ import { attachInPty, cleanEnv, preflight, + readScreen, recordEvents, renderScreen, startBroker, @@ -60,12 +61,7 @@ async function screenAfterStatusPaints(opts: { await write(`\x1b[${opts.rows - 4};1H`); for (let n = 0; n < opts.repaints; n += 1) await write(opts.render(n)); - const buf = term.buffer.active; - const screen: string[] = []; - for (let y = 0; y < opts.rows; y += 1) { - screen.push((buf.getLine(y)?.translateToString(true) ?? '').trimEnd()); - } - return screen; + return readScreen(term, opts.rows); } describe('attach status line survives a narrow pane', () => { @@ -140,6 +136,13 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay let broker: BrokerHandle; let events: EventRecorder; const clients = new Map(); + /** + * Each pane's capture size once its attach snapshot had landed but before + * the game got going — the baseline the live-stream assertion grows from. + * A fixed byte floor would be a guess about snapshot size, which varies with + * pane geometry and harness banner. + */ + const afterSnapshot = new Map(); beforeAll(async () => { tmpRoot = mkdtempSync(path.join(os.tmpdir(), 'relay-ttt-')); @@ -173,6 +176,16 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay ); } + // Let every pane paint its attach snapshot, then record that as the + // baseline. `view` writes the snapshot in one shot on connect, so a pane + // that never grows past this is the frozen-stream failure being guarded. + await waitFor(() => AGENTS.every((n) => clients.get(n)!.size() > 0), { + timeoutMs: 60_000, + label: 'every view client to paint its snapshot', + intervalMs: 500, + }); + for (const name of AGENTS) afterSnapshot.set(name, clients.get(name)!.size()); + // The game is three LLM agents taking turns over the network; give it room. // // Wait for DONE_TOKEN to be DELIVERED to a player. The token is named only @@ -187,10 +200,13 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay ); }, 10 * 60_000); - afterAll(() => { + afterAll(async () => { events?.stop(); for (const client of clients.values()) client.stop(); - broker?.stop(); + // Await the shutdown before removing the tree: `node down` reads the + // broker's connection file out of it, so deleting first would strand the + // broker and its Claude workers. + await broker?.stop(); engine?.stop(); if (tmpRoot && existsSync(tmpRoot)) rmSync(tmpRoot, { recursive: true, force: true }); }); @@ -198,10 +214,12 @@ describe.skipIf(!liveEnabled)('three PTY agents play tic-tac-toe over the relay it('streams live PTY output to every view client', () => { for (const name of AGENTS) { // A frozen `view` pane — the original report — shows up here as a - // capture no bigger than the one-shot attach snapshot. - expect(clients.get(name)!.size(), `${name} view stream never grew past its snapshot`).toBeGreaterThan( - 20_000 - ); + // capture still the size of its one-shot attach snapshot. + const baseline = afterSnapshot.get(name)!; + expect( + clients.get(name)!.size(), + `${name} view stream never grew past its ${baseline}-byte snapshot` + ).toBeGreaterThan(baseline); } }); From d5ce5d8631bd66ea6d14f7d641f18c84452fca85 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 20:47:49 +0000 Subject: [PATCH 8/8] test(e2e): rebuild the broker url from a host literal and integer port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loopback validator still interpolated the file's own characters, so CodeQL kept flagging the two `fetch` call sites as "file data in outbound network request" — correctly: the value was inspected, not untainted. It now picks the host from a fixed allow-list and parses the port as a range-checked integer, so the returned origin is assembled from a literal and a number rather than from anything the file supplied. Adds tests covering the normalize case (path, query, and credentials dropped) and the rejections: non-http scheme, remote host, missing port, out-of-range port, unparseable. Verified: e2e suite 17/17 including the live game; prettier and tsc clean; eslint at the 41-warning baseline. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D2zCQ1UfekqREYCKfhF9SF --- tests/e2e/tic-tac-toe/harness.ts | 40 ++++++++++++------- tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts | 22 ++++++++++ 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/tests/e2e/tic-tac-toe/harness.ts b/tests/e2e/tic-tac-toe/harness.ts index cc76e7c2e..8d5115a75 100644 --- a/tests/e2e/tic-tac-toe/harness.ts +++ b/tests/e2e/tic-tac-toe/harness.ts @@ -157,16 +157,28 @@ export async function startEngine(serveBin: string, tmpRoot: string): Promise { + throw new Error(`${source}: refusing to use broker url — ${why}`); + }; + let url: URL; try { url = new URL(String(raw)); @@ -174,16 +186,15 @@ export function requireLoopbackUrl(raw: unknown, source: string): string { return fail(`not a URL: ${JSON.stringify(raw)}`); } if (url.protocol !== 'http:') return fail(`expected http:, got ${url.protocol}`); - if (!['127.0.0.1', 'localhost', '[::1]', '::1'].includes(url.hostname)) { - return fail(`expected a loopback host, got ${url.hostname}`); - } - if (!url.port) return fail('expected an explicit port'); - // Rebuilt from validated parts — no path, query, or credentials survive. - return `http://${url.hostname}:${url.port}`; - function fail(why: string): never { - throw new Error(`${source}: refusing to use broker url — ${why}`); + const host = LOOPBACK_HOSTS.find((candidate) => candidate === url.hostname); + if (!host) return fail(`expected a loopback host, got ${url.hostname}`); + + const port = Number.parseInt(url.port, 10); + if (!Number.isInteger(port) || port < 1 || port > 65535) { + return fail(`expected a port in 1-65535, got ${JSON.stringify(url.port)}`); } + return `http://${host}:${port}`; } export interface BrokerHandle { @@ -413,11 +424,10 @@ export interface EventRecorder { export async function recordEvents(brokerUrl: string, apiKey: string): Promise { const { default: WebSocket } = await import('ws'); // Re-validate: `recordEvents` is exported, so it must not assume its caller - // already pinned the origin to loopback. - const origin = new URL(requireLoopbackUrl(brokerUrl, 'recordEvents')); - const socket = new WebSocket(`ws://${origin.hostname}:${origin.port}/ws`, { - headers: { 'X-API-Key': apiKey }, - }); + // already pinned the origin to loopback. The result is rebuilt from a host + // literal and an integer port, so swapping the scheme keeps it untainted. + const origin = requireLoopbackUrl(brokerUrl, 'recordEvents').replace('http://', 'ws://'); + const socket = new WebSocket(`${origin}/ws`, { headers: { 'X-API-Key': apiKey } }); const messages: RelayMessage[] = []; // A Map, not an object: `kind` is an arbitrary string off the wire, and diff --git a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts index 770270e36..821bd9bd6 100644 --- a/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts +++ b/tests/e2e/tic-tac-toe/tic-tac-toe-e2e.test.ts @@ -33,6 +33,7 @@ import { cleanEnv, preflight, readScreen, + requireLoopbackUrl, recordEvents, renderScreen, startBroker, @@ -121,6 +122,27 @@ describe('attach status line survives a narrow pane', () => { }); }); +describe('requireLoopbackUrl', () => { + it('normalizes a valid loopback broker url', () => { + expect(requireLoopbackUrl('http://127.0.0.1:43719', 'test')).toBe('http://127.0.0.1:43719'); + // Path, query, and credentials are dropped by the rebuild. + expect(requireLoopbackUrl('http://localhost:8080/api?x=1', 'test')).toBe('http://localhost:8080'); + }); + + // A stale or tampered `connection.json` must not be able to point the + // harness — which attaches an API key to every request — at another host. + it.each([ + ['https://127.0.0.1:443', 'non-http scheme'], + ['http://evil.example.com:80', 'remote host'], + ['http://127.0.0.1', 'no port'], + ['http://127.0.0.1:0', 'port out of range'], + ['http://127.0.0.1:99999', 'port out of range'], + ['not a url', 'unparseable'], + ])('rejects %s (%s)', (raw) => { + expect(() => requireLoopbackUrl(raw, 'test')).toThrow(/refusing to use broker url/); + }); +}); + // ── the live game ──────────────────────────────────────────────────────────── const check = preflight();