diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b513857..0555c61 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -38,22 +38,268 @@ It does **not** analyze project code or prevent concurrent write conflicts insid ## 4. Instance lifecycle & reconnect semantics - An instance is a server-managed process; UI windows are merely views. - Default interactive path is WebSocket TTY: `GET /api/instances/tty/ws?id=...` (bi-directional terminal stream). - - **Handshake Protocol**: Server sends `{"type":"ready"}` on connect; client must wait for this before sending resize to start data flow. - - **Timeout Handling**: Client should implement 5s handshake timeout with SSE fallback. - - **Resize Support**: Client sends `{"type":"resize","cols":80,"rows":24}` to update PTY size, triggering TUI programs to redraw. - - **Client-side Config**: Terminal buffer size (scrollback) is configurable on the client side to control how much history is retained in memory for scrolling. This is a client-side setting and does not affect server-side log persistence. - Fallback path remains available: HTTP input `POST /api/instances/input` + replay/SSE logs (`GET /api/instances/log`, `GET /api/instances/log/stream`). - UI shows transport state (`websocket/sse/polling`) and supports manual WS reconnect. - Backlog is stored on disk with a size cap (rolling truncate). - On server startup, stale persisted `running` records are reconciled to `stopped` because in-memory stdin/stdout bindings cannot be resumed after process restart. -## 5. Security model (single-user) +## 5. Terminal Protocol Timing Specification + +This section defines the strict timing protocol for terminal I/O to prevent escape sequence leakage and ensure reliable data flow. + +### 5.1 Terminal Connection State Machine + +The terminal connection goes through the following states: + +``` +IDLE → CONNECTING → HANDSHAKING → READY → DISCONNECTING → IDLE +``` + +| State | Description | Allowed Actions | +|-------|-------------|-----------------| +| IDLE | No active connection | May initiate connection | +| CONNECTING | WebSocket opening in progress | None (wait) | +| HANDSHAKING | WebSocket open, waiting for server `ready` message | None (wait) | +| READY | Connection fully established | Send input, receive output, resize | +| DISCONNECTING | Connection closing in progress | None (wait) | + +### 5.2 WebSocket Handshake Protocol + +**Server → Client (on WebSocket open):** +```json +{"type": "ready"} +``` + +The client MUST wait for this message before: +- Sending any input data +- Sending resize commands +- Focusing the terminal (to prevent query sequence leakage) + +**Client → Server (after receiving `ready`):** +```json +{"type": "resize", "cols": 80, "rows": 24} +``` + +**Timeout:** Client MUST implement a 5-second handshake timeout. If `ready` is not received, fall back to SSE mode. + +### 5.3 Instance Switch Protocol + +When switching between instances (worktree or instance tabs), the following sequence MUST be followed strictly: + +``` +Phase 1: Disconnect Previous +├── 1.1 Call disconnectTTY() - close old WebSocket +├── 1.2 Reset logCursor = 0 +└── 1.3 Clear terminal buffers (if terminal exists) + ├── For running instances: + │ ├── Write '\x1b[?1049l' (exit alternate buffer) + │ ├── Write '\x1b[2J\x1b[H' (clear normal buffer, home cursor) + │ └── Write '\x1b[?1049h' (re-enter alternate buffer, now clean) + └── For stopped instances: + ├── Call term.reset() + └── Write '\x1b[2J\x1b[3J\x1b[H' (clear screen and scrollback) + +Phase 2: Load Initial Data +├── 2.1 If stopped: loadLog() once, update status to "stopped", DONE +└── 2.2 If running: loadLog() then proceed to Phase 3 + +Phase 3: Establish New Connection (running instances only) +├── 3.1 Call connectTTY() +│ ├── Create new WebSocket +│ ├── Set state to CONNECTING +│ └── Register onopen handler +├── 3.2 Wait for WebSocket.onopen +├── 3.3 Wait for server {"type": "ready"} message +├── 3.4 Set state to READY +├── 3.5 Send initial resize: {"type": "resize", "cols": N, "rows": M} +└── 3.6 NOW and ONLY NOW: Call focusTerminalIfPossible() +``` + +**Critical Timing Rule:** The terminal MUST NOT receive focus until Phase 3.6 (after `ready` message is received and resize is sent). + +### 5.4 Focus Management Rules + +The terminal focus is managed by `focusTerminalIfPossible()` which checks: + +```javascript +function focusTerminalIfPossible() { + if (!term) return; // No terminal instance + if (!windowHasFocus) return; // Browser window not focused + if (openDialogs.size > 0) return; // Modal dialog is open + if (!state.activeInst) return; // No active instance + if (ttyState !== 'READY') return; // ⚠️ Connection not ready + const inst = state.instances.find(i => i.id === state.activeInst); + if (inst && inst.status !== 'running') return; // Instance not running + + term.focus(); +} +``` + +**The `ttyState` check is CRITICAL** - it prevents focus before the WebSocket is ready, which would cause xterm.js to send terminal query sequences (OSC 11, DA, DEC Private Mode queries) before the data path is established. + +### 5.5 Terminal Query Sequence Prevention + +When xterm.js gains focus, it sends query sequences to detect terminal capabilities: +- `OSC 11` - Background color query → Response: `\x1b]11;rgb:...` +- `OSC 4` - Palette color query → Response: `\x1b]4;...;rgb:...` +- `DA (Device Attributes)` → Response: `\x1b[?1;2c` +- `DEC Private Mode Report` → Response: `\x1b[?2027;0$y...` + +If these responses arrive before the WebSocket is in READY state, they may be incorrectly routed or displayed as garbage text. + +**Prevention Strategy:** +1. Never focus terminal before WebSocket is READY +2. Always complete handshake before sending resize +3. Send resize before focusing (this triggers any needed redraw) + +### 5.6 Error Handling & Recovery + +| Scenario | Detection | Recovery Action | +|----------|-----------|-----------------| +| Handshake timeout | No `ready` in 5s | Close WS, fallback to SSE | +| WebSocket close | onclose event | Retry after 1s delay | +| Instance stopped | status !== 'running' | Disconnect WS, show log with banner | +| Browser blur | blur event | Track windowHasFocus = false | +| Dialog open | showModal intercept | Track openDialogs.add() | + +### 5.7 Protocol Message Summary + +**Server → Client Messages:** + +| Message | Format | When | +|---------|--------|------| +| Ready | `{"type":"ready"}` | Immediately after WebSocket opens | +| Output (binary) | `ArrayBuffer` | PTY output available | +| Output (text) | `string` | PTY output available | + +**Client → Server Messages:** + +| Message | Format | When | +|---------|--------|------| +| Resize | `{"type":"resize","cols":N,"rows":M}` | After `ready`, on terminal resize | +| Input | raw string/bytes | User keystroke (anytime after `ready`) | + +### 5.8 Implementation Checklist + +When implementing or modifying terminal connection code, verify: + +- [ ] `focusTerminalIfPossible()` checks connection state before focusing +- [ ] WebSocket handshake timeout (5s) is implemented +- [ ] `ready` message triggers resize BEFORE focus +- [ ] Instance switch follows Phase 1 → 2 → 3 sequence +- [ ] Dialog close delays focus until connection is ready +- [ ] Window focus event respects connection state + +### 5.9 Terminal Query Response Filtering + +#### Background & Problem Statement + +When xterm.js receives terminal query sequences (e.g., `ESC[c` for Device Attributes), it generates responses (e.g., `ESC[?1;2c`) and sends them via the `term.onData()` callback. These responses are intended to be forwarded to the PTY, which then interprets them. + +However, in certain scenarios, these responses may: +1. Be displayed as garbage text in the terminal +2. Confuse shell programs (zsh, bash) that receive unexpected input +3. Appear as split sequences due to data fragmentation + +#### Observed Behavior + +After a TUI program (like `opencode` CLI) exits and returns to the shell prompt: +- The shell may send terminal capability queries +- xterm.js responds to these queries +- The responses are sent to PTY via `term.onData()` +- The shell displays these responses as text: `;1R`, `rgb:0b0b/1010/2020`, `?2027;0$y` + +#### Root Cause Analysis + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ Data Flow Diagram │ +├─────────────────────────────────────────────────────────────────────┤ +│ │ +│ [Shell in PTY] │ +│ │ │ +│ │ sends query: ESC[c (Device Attributes) │ +│ ▼ │ +│ [PTY stdout] ──► [WebSocket] ──► [xterm.js term.write()] │ +│ │ │ +│ │ xterm.js generates │ +│ │ response: ESC[?1;2c │ +│ ▼ │ +│ [term.onData()] │ +│ │ │ +│ │ forwarded back │ +│ ▼ │ +│ [Shell in PTY] ◄── [WebSocket] ◄── [SendInput] │ +│ │ │ +│ │ shell receives unexpected input │ +│ ▼ │ +│ [Displayed as garbage text] │ +│ │ +└─────────────────────────────────────────────────────────────────────┘ +``` + +The issue occurs because: +1. Shell sends terminal query to check capabilities (normal behavior) +2. xterm.js receives query and generates response +3. Response is forwarded back to PTY via `term.onData()` → `SendInput()` +4. Shell receives response as keyboard input and displays it + +#### Current Mitigation + +The frontend implements a response filter in `term.onData()`: + +```javascript +term.onData(data => { + // Filter out terminal query responses + if (isTerminalQueryResponse(data)) { + console.debug('Filtered terminal response:', data.length, 'bytes'); + return; + } + // ... forward to WebSocket +}); +``` + +The `isTerminalQueryResponse()` function detects: +- Complete sequences with ESC prefix: `ESC]11;rgb:...`, `ESC[?1;2c`, `ESC[?2027;0$y` +- Partial sequences (split or fragmented): `;1R`, `rgb:...`, `?2027;0$y` + +#### Known Limitations & Risks + +**⚠️ This implementation has limitations documented in `docs/TERMINAL_FILTER_REVIEW.md`:** + +1. **Cannot distinguish user intent**: If a user intentionally sends `ESC[c` to query terminal attributes, it will be filtered as well. + +2. **Regex limitations**: CSI patterns may have edge cases not covered. + +3. **False positives on small coordinates**: Cursor position reports with small coordinates may be incorrectly filtered. + +4. **Incomplete coverage**: Some terminal response types (DSR, etc.) are not explicitly handled. + +5. **Architecture consideration**: The filter is placed in the input path (`term.onData`), which handles user keystrokes. See the review document for detailed analysis of input vs output path considerations. + +#### Future Considerations + +If issues arise with the current filtering approach: +1. Refer to `docs/TERMINAL_FILTER_REVIEW.md` for detailed analysis +2. Consider alternative approaches: + - Move filtering to the output path (log filtering) + - Configure xterm.js to suppress automatic query responses + - Add user intent detection (keyed sequences vs automatic responses) +3. Update `docs/TERMINAL_IO_ANALYSIS.md` with any new findings + +#### Related Documents + +- `docs/TERMINAL_FILTER_REVIEW.md` - Detailed review of current implementation +- `docs/TERMINAL_IO_ANALYSIS.md` - Terminal I/O architecture and filtering +- `docs/TERMINAL_TEST_CASES.md` - Test cases for terminal behavior + +## 6. Security model (single-user) - Default listen: loopback only. - Non-loopback requires `--auth`. - Optional built-in HTTPS via `--tls-cert/--tls-key`. - Origin/Host check + basic rate limit on unauthorized attempts. - Redaction on stored backlog (e.g. `sk-...`). -## 6. MCP extensibility +## 7. MCP extensibility - Core managers (worktree/instance) are transport-agnostic. - `internal/mcp` exposes tool names; server dispatch maps tool calls to existing core managers without rewriting core. diff --git a/docs/TERMINAL_FILTER_REVIEW.md b/docs/TERMINAL_FILTER_REVIEW.md new file mode 100644 index 0000000..6cd4780 --- /dev/null +++ b/docs/TERMINAL_FILTER_REVIEW.md @@ -0,0 +1,400 @@ +# Terminal Filter Review Report + +**Date**: 2026-03-13 +**Review Target**: Uncommitted changes to `internal/ui/static/index.html` +**Related Documents**: `TERMINAL_IO_ANALYSIS.md`, `TERMINAL_TEST_CASES.md`, `ARCHITECTURE.md` + +--- + +## Executive Summary + +This report analyzes the uncommitted changes that add terminal query response filtering logic to the frontend. While the intent is correct (preventing terminal query responses from being sent to PTY), the implementation has **significant logical contradictions** with the documented architecture and may cause false positives. + +### Risk Level: ⚠️ MEDIUM + +--- + +## 1. Change Overview + +### Modified File +- `internal/ui/static/index.html` (JavaScript embedded in HTML) + +### Changes Added +1. New function `isTerminalQueryResponse(data)` (~50 lines) +2. Filter call in `term.onData()` callback +3. State management enhancements (ttyState reset, reconnection logic) + +### Stated Purpose +Filter out terminal query responses generated by xterm.js that should not be sent to PTY as they confuse shell programs. + +--- + +## 2. Architecture Contradiction Analysis + +### 2.1 Input Path vs Output Path Confusion + +**Documented Architecture** (`TERMINAL_IO_ANALYSIS.md` - Architecture Flow): + +``` +Input Path: +User Input (Browser) → WebSocket/API → SendInput() → PTY stdin + +Output Path: +Program stdout/stderr → PTY → pumpLogs() → redact.Text() → Log file + WebSocket → Browser xterm.js +``` + +**Problem**: The filtering logic is placed in `term.onData()`, which handles **user input**. However, terminal query responses are generated by xterm.js automatically when it receives query sequences from the **output** stream. + +**Key Question**: What data is actually being filtered? + +| Scenario | Data Source | Should Filter? | Current Implementation | +|----------|-------------|----------------|------------------------| +| User types `ESC[c` manually | User input | NO (intentional) | ❌ Would filter | +| xterm.js auto-generates response | Internal | YES | ❌ Never reaches `onData` | +| Shell script sends query | Program output → user sees it | NO (display only) | ❌ N/A | + +**Conclusion**: The `term.onData()` callback is triggered by **user keystrokes**, not by xterm.js internal responses. Terminal query responses generated by xterm.js are written to the display buffer via `term.write()`, not sent back through `onData`. + +--- + +### 2.2 ESC Prefix Handling Contradiction + +**Backend Documented Principle** (`TERMINAL_IO_ANALYSIS.md` §3.2): + +> **Does NOT match** (preserve these): +> - Legitimate ESC sequences (have 0x1B byte) + +The backend filtering logic in `redact.go` explicitly **preserves** sequences with the ESC (0x1B) byte, only filtering **residues** that are missing the ESC prefix. + +**Frontend Implementation**: + +```javascript +if (data.charCodeAt(0) !== 0x1b) { + // Handle no-ESC case + // ... + return false; +} +// Has ESC - check type +if (data[1] === ']' || data[1] === 'P') return true; // FILTER +if (data[1] === '[' && data.length > 2 && data[2] === '?') { + // Filter DEC private mode responses + return true; +} +``` + +**Contradiction**: The frontend **filters** sequences WITH the ESC prefix, while the backend **preserves** them. + +**Concrete Examples**: + +| Input Data | Backend (`redact.go`) | Frontend (`isTerminalQueryResponse`) | Result | +|------------|----------------------|-------------------------------------|--------| +| `ESC]0;My Terminal BEL` | ✓ Preserved (legal OSC) | ❌ **Filtered** (`data[1] === ']'`) | **False Positive** | +| `ESC[?1000h` | ✓ Preserved (mouse enable) | ✓ Preserved (no match) | Correct | +| `ESC[?1;2c` | ✓ Preserved (device attributes) | ❌ **Filtered** (matches `/\?[\d;]+c$/`) | **False Positive** | +| `ESC[35;107R` | ✓ Preserved (cursor position) | ❌ **Filtered** (matches cursor R pattern) | **False Positive** | + +--- + +## 3. Coverage Gap Analysis + +### 3.1 Terminal Response Types (per ECMA-48 / XTerm CTLSEQS) + +| Response Type | Format | Terminator | Covered? | +|--------------|--------|------------|----------| +| **OSC Responses** | `ESC]...BEL` or `ESC]\` | BEL (0x07) or ST | ⚠️ Partial | +| **DCS Responses** | `ESCP...ST` | ST (0x1B 0x5C) | ⚠️ Partial | +| **CSI Device Attributes** | `ESC[?params c` | `c` | ✓ | +| **CSI Cursor Position** | `ESC[row;col R` | `R` | ✓ | +| **CSI DEC Private Mode Report** | `ESC[?params $y` | `$y` | ✓ | +| **CSI DSR (Device Status)** | `ESC[params n` | `n` | ❌ **Missing** | +| **CSI Terminal State Dump** | `ESC[params x` | `x` | ❌ **Missing** | +| **CSI Soft Reset** | `ESC[!p` | `p` | ❌ **Missing** | + +### 3.2 Specific Gaps + +#### Gap 1: DSR (Device Status Report) Responses + +Standard DSR responses use terminator `n`: +- `ESC[0n` - Terminal OK (no malfunction) +- `ESC[3n` - Terminal malfunction +- `ESC[?10;1n` - DEC custom response + +**Current code**: No handling for `n` terminator. + +#### Gap 2: Incomplete OSC Response Detection + +Current code: +```javascript +if (/^rgb:/i.test(data)) return true; +``` + +**Problems**: +1. Only checks prefix, not full format +2. Missing other OSC response types: + - `ESC]4;palette;rgb:1234/5678/9abc` + - `ESC]10;foreground;rgb:...` + - `ESC]11;background;rgb:...` + - `ESC]12;cursor;rgb:...` + +#### Gap 3: Regex Error in Cursor Position Filter + +Current code: +```javascript +if (data[1] === '[' && /\[\d+;\d+R$/i.test(data)) return true; +``` + +**Problem**: The regex `/\[\d+;\d+R$/` expects a literal `[` character, but `data[1] === '['` already confirms the second character is `[`. This means the regex is looking for `[[` (double bracket), which is incorrect. + +**Correct pattern should be**: `/^\[\d+;\d+R$/i` (matching from position 0 after the ESC) or the check should be against `data.substring(1)`. + +--- + +## 4. Overlap with Backend Filtering + +### Backend Already Filters Residues + +Per `TERMINAL_IO_ANALYSIS.md` §2.3 and §3.1, the backend `redact.Text()` function filters: + +| Pattern | Example | Backend Action | +|---------|---------|----------------| +| Mouse residue (no ESC) | `35;107;1M` | ❌ Filtered | +| Cursor residue (no ESC) | `35;107R` | ❌ Filtered | +| Partial prefix residue | `[<0;100;50M` | ❌ Filtered | + +### Frontend Redundant Filtering + +The frontend attempts to filter **complete sequences with ESC**, while the backend filters **incomplete sequences without ESC**. + +**Risk**: If a complete sequence somehow reaches the backend (e.g., program legitimately outputs `ESC[?1;2c`), the backend preserves it. But if the user's input contains the same sequence, the frontend filters it. This creates **inconsistent behavior** between input and output paths. + +--- + +## 5. Edge Case Analysis + +### 5.1 User Intentionally Sends Query Sequences + +**Scenario**: A user is testing terminal behavior or running a script that sends `ESC[c` to query device attributes. + +**Expected**: The sequence should be sent to PTY. + +**Actual**: Filtered by `isTerminalQueryResponse()`. + +**Impact**: Breaks legitimate use cases for terminal introspection and testing. + +### 5.2 Small Terminal Coordinates + +Per `TERMINAL_IO_ANALYSIS.md` §4.2, the backend intentionally does NOT filter coordinates where all params < 10 to avoid false positives. + +**Frontend code**: +```javascript +if (/^;?\d+(;\d+)?R$/i.test(data)) return true; +``` + +**Problem**: This matches `[5;10R` (small terminal cursor report) which the backend explicitly preserves. + +**Inconsistency**: Frontend is more aggressive than backend. + +### 5.3 Incomplete Sequences + +Per `TERMINAL_IO_ANALYSIS.md` §4.3, incomplete sequences like `35;107` (missing terminator) are NOT filtered. + +**Frontend code**: Does not handle incomplete sequences (correct), but also doesn't document this design decision. + +--- + +## 6. State Management Changes Review + +### 6.1 ttyState Reset on Instance Stop + +**Change**: +```javascript +if (inst && inst.status !== 'running') { + ttyState = 'IDLE'; // Reset state before returning + updateStatus("stopped"); + return; +} +``` + +**Assessment**: ✅ **Correct**. Ensures state machine returns to IDLE when instance stops. + +### 6.2 Reconnection Logic Enhancement + +**Change**: +```javascript +if (ttySocket === ws) { + const inst = state.instances.find(i => i.id === state.activeInst); + if (inst && inst.status === 'running') { + updateStatus("ws closed, retrying..."); + ttyReconnectTimer = setTimeout(connectTTY, 1000); + } else { + updateStatus("stopped"); + } +} +``` + +**Assessment**: ✅ **Correct**. Prevents reconnection attempts for stopped instances. + +### 6.3 Comment Update + +**Change**: +```javascript +// Before: For polling state transitions +// After: For waiting state transitions +``` + +**Assessment**: ✅ **Neutral**. Minor clarification, no functional impact. + +--- + +## 7. Compliance Check + +### 7.1 ARCHITECTURE.md §5 (Terminal Protocol) + +| Requirement | Compliance | Notes | +|-------------|------------|-------| +| State machine (IDLE→CONNECTING→HANDSHAKING→READY→DISCONNECTING) | ✅ | Implemented | +| `focusTerminalIfPossible()` checks `ttyState === 'READY'` | ✅ | Line 955 | +| WebSocket handshake protocol | ✅ | Conforms to §5.2 | +| Instance switch protocol (Phase 1→2→3) | ✅ | Conforms to §5.3 | +| State reset on instance stop | ✅ | Added `ttyState = 'IDLE'` | + +### 7.2 PRD.md + +| Requirement | Compliance | Notes | +|-------------|------------|-------| +| Terminal instances persist independently of frontend | ✅ | Reconnection logic enhanced | +| Web TTY support | ✅ | Conforms to §7 | + +### 7.3 API.md §4 (WebSocket TTY) + +| Aspect | Compliance | Notes | +|--------|------------|-------| +| Handshake protocol (`{"type":"ready"}`) | ✅ | Unchanged | +| Message types | ✅ | Unchanged | +| Timeout & fallback | ✅ | Unchanged | + +**Note**: API.md does not mention client-side filtering. This is an internal optimization, not an API change. + +--- + +## 8. Recommendations + +### 8.1 Critical Fixes + +1. **Remove or relocate filtering logic** + - Terminal query responses are generated by xterm.js internally, not via `onData` + - Current placement filters user input, not xterm.js responses + - Consider removing entirely or moving to output path (`term.write()` interceptor) + +2. **Fix ESC prefix contradiction** + - If sequence has ESC, it should be PRESERVED (per backend principle) + - Only filter residues WITHOUT ESC prefix + +3. **Fix regex error** + - `/\[\d+;\d+R$/i` should be `/^\[\d+;\d+R$/i` or applied to `data.substring(1)` + +### 8.2 Coverage Improvements + +4. **Add missing terminators** + - `n` for DSR responses + - `x` for terminal state dump + - `p` for soft reset + +5. **Improve OSC detection** + - Validate full format, not just `rgb:` prefix + - Handle all OSC response types (palette, foreground, background, cursor) + +### 8.3 Consistency Improvements + +6. **Align with backend filtering principles** + - Only filter residues (no ESC prefix) + - Preserve small coordinates (< 10) to avoid false positives + - Document incomplete sequence handling + +7. **Add test cases** + - Mirror `TERMINAL_TEST_CASES.md` structure for frontend + - Include false positive tests (user intentional input) + - Test all response types from §3.1 + +### 8.4 Documentation Updates + +8. **Update `TERMINAL_IO_ANALYSIS.md`** + - Document frontend filtering separately from backend + - Clarify input path vs output path responsibilities + - Add section on terminal query response handling + +--- + +## 9. Conclusion + +### Summary Table + +| Aspect | Status | Risk | +|--------|--------|------| +| Filtering placement (`onData`) | ❌ Incorrect | HIGH | +| ESC prefix handling | ❌ Contradicts backend | HIGH | +| Regex correctness | ❌ Error in cursor pattern | MEDIUM | +| Coverage completeness | ⚠️ Partial | MEDIUM | +| State management changes | ✅ Correct | LOW | +| Reconnection logic | ✅ Correct | LOW | +| Documentation compliance | ✅ ARCHITECTURE.md | LOW | + +### Overall Assessment + +The **state management changes** (ttyState reset, reconnection logic) are correct and should be retained. + +The **filtering logic** has fundamental architectural issues: +1. Placed in wrong data path (input vs output) +2. Contradicts documented backend filtering principles +3. Contains regex errors +4. Incomplete coverage of terminal response types + +### Recommended Action + +**Do not commit the filtering logic in its current form.** Either: +1. **Remove** the filtering entirely and rely on backend filtering +2. **Relocate** to the output path and fix the ESC prefix logic +3. **Document** the design decision and add comprehensive tests + +--- + +## Appendix A: Terminal Response Type Reference + +| Type | Format | Example | Should Filter? | +|------|--------|---------|----------------| +| OSC Color Query Response | `ESC]11;rgb:R/G/B BEL` | `]11;rgb:1234/5678/9abc` | Context-dependent | +| DSR OK | `ESC[0n` | `[0n` | NO (too short) | +| DSR Malfunction | `ESC[3n` | `[3n` | NO (too short) | +| Device Attributes | `ESC[?1;2c` | `[?1;2c` | Context-dependent | +| Cursor Position | `ESC[row;colR` | `[35;107R` | If residue (no ESC) | +| DEC Mode Report | `ESC[?params$y` | `[?2027;0$y` | If residue (no ESC) | +| SGR Mouse Event | `ESC[Start Instance let fitAddon = null; let windowHasFocus = true; let openDialogs = new Set(); + let ttyState = 'IDLE'; // IDLE | CONNECTING | HANDSHAKING | READY | DISCONNECTING + let ttyStateCheckInterval = null; // For waiting state transitions function focusTerminalIfPossible() { if (!term) return; if (!windowHasFocus) return; if (openDialogs.size > 0) return; if (!state.activeInst) return; + if (ttyState !== 'READY') return; // CRITICAL: Only focus when connection is ready const inst = state.instances.find(i => i.id === state.activeInst); if (inst && inst.status !== 'running') return; @@ -1004,6 +1007,39 @@

Start Instance

const inst = state.instances.find(i => i.id === state.activeInst); if (inst && inst.status !== 'running') return; // Disable input if not running + // ===================================================================== + // Terminal Query Response Filtering + // ===================================================================== + // + // Background: When xterm.js receives terminal query sequences (e.g., + // ESC[c for Device Attributes), it generates responses and sends them + // via this onData() callback. If forwarded to PTY, these responses + // appear as garbage text in the shell. + // + // Problem: After TUI programs exit, the shell may send capability + // queries. xterm.js responds, and the response is displayed as + // unwanted text like: ;1R, rgb:0b0b/1010/2020, ?2027;0$y + // + // Current Solution: Filter detected responses before forwarding. + // + // Known Limitations (see docs/TERMINAL_FILTER_REVIEW.md): + // 1. Cannot distinguish user intent - user's intentional ESC sequences + // (e.g., scripts testing terminal) may be incorrectly filtered. + // 2. Regex may miss edge cases or cause false positives. + // 3. Small coordinate reports may be incorrectly filtered. + // 4. Some response types (DSR, etc.) not explicitly covered. + // 5. Filter is in input path; see review doc for alternative approaches. + // + // If issues arise, refer to: + // - docs/TERMINAL_FILTER_REVIEW.md for detailed analysis + // - docs/TERMINAL_IO_ANALYSIS.md for architecture + // ===================================================================== + + if (isTerminalQueryResponse(data)) { + console.debug('Filtered terminal response:', data.length, 'bytes'); + return; + } + if (ttySocket && ttySocket.readyState === WebSocket.OPEN) { ttySocket.send(data); } else { @@ -1023,6 +1059,68 @@

Start Instance

}); } + // Minimum length for terminal response detection + const MIN_RESPONSE_LENGTH = 2; + + // ===================================================================== + // Terminal Query Response Detection + // ===================================================================== + // + // Detects xterm.js auto-generated responses that should NOT be forwarded + // to the PTY. These responses are generated when xterm.js receives + // terminal query sequences. + // + // Response Types Handled: + // - OSC responses: ESC]11;rgb:... (background color), ESC]4;... (palette) + // - CSI responses: ESC[?1;2c (Device Attributes), ESC[?2027;0$y (DEC mode) + // - Cursor Position: ESC[row;colR or fragment ;colR + // - Partial fragments: Missing ESC prefix due to data split + // + // CRITICAL LIMITATIONS: + // - User-sent sequences matching these patterns WILL BE FILTERED + // - Some legitimate sequences may be caught (false positives) + // - See docs/TERMINAL_FILTER_REVIEW.md for complete analysis + // + // DO NOT MODIFY without reviewing: + // - docs/TERMINAL_FILTER_REVIEW.md (Section 3: Coverage Gap Analysis) + // - docs/TERMINAL_IO_ANALYSIS.md (Section 2.2: ESC Prefix Handling) + // ===================================================================== + + function isTerminalQueryResponse(data) { + if (!data || data.length < MIN_RESPONSE_LENGTH) return false; + + // Check for ESC prefix + if (data.charCodeAt(0) !== 0x1b) { + // No ESC - check for partial response patterns (missing ESC[ prefix) + // Pattern: ;1R, 1;1R, ;1;1R (cursor position report suffix) + // These appear when ESC[ is stripped from the response + if (/^;?\d+(;\d+)?R$/i.test(data)) return true; + // Pattern: ?1;1$y (DEC mode report suffix) + if (/^\?\d+(;\d+)*\$y$/i.test(data)) return true; + // Pattern: rgb:hex/hex/hex (OSC color response suffix) + if (/^rgb:/i.test(data)) return true; + return false; + } + + // OSC responses: \x1b]... (color queries, etc.) + if (data[1] === ']' || data[1] === 'P') return true; + + // CSI responses starting with ? (private mode reports) + if (data[1] === '[' && data.length > 2 && data[2] === '?') { + // DEC private mode report: ?...$y + if (/\$\d*y$/.test(data)) return true; + // Device attributes: ?...c + if (/\?[\d;]+c$/.test(data)) return true; + // Cursor position report: ?...R + if (/\?[\d;]+R$/i.test(data)) return true; + } + + // Standard CSI cursor position: \x1b[row;colR + if (data[1] === '[' && /\[\d+;\d+R$/i.test(data)) return true; + + return false; + } + // Auto-focus terminal when window gains focus and no dialogs are open window.addEventListener('focus', () => { windowHasFocus = true; @@ -1102,18 +1200,64 @@

Start Instance

} function disconnectTTY() { + // Clear any pending state check interval + if (ttyStateCheckInterval) { + clearInterval(ttyStateCheckInterval); + ttyStateCheckInterval = null; + } + // Set DISCONNECTING if there's an active socket, otherwise IDLE + // The onclose handler will transition to IDLE after socket closes + if (ttySocket) { + ttyState = 'DISCONNECTING'; + } else { + ttyState = 'IDLE'; + } if (ttyReconnectTimer) clearTimeout(ttyReconnectTimer); if (logStream) { logStream.close(); logStream = null; } if (ttySocket) { ttySocket.close(); ttySocket = null; } } function connectTTY() { + // Phase 1: Disconnect Previous (already done in disconnectTTY) disconnectTTY(); + + // Phase 3.1: Validate state before establishing new connection + // Must be IDLE before transitioning to CONNECTING (per §5.3) + if (ttyState !== 'IDLE') { + console.warn('connectTTY: invalid state transition from', ttyState, '- waiting for IDLE'); + // Wait for state to become IDLE before proceeding, with max 10 retries (1 second) + let retries = 0; + const maxRetries = 10; + ttyStateCheckInterval = setInterval(() => { + retries++; + if (ttyState === 'IDLE' || ttyState === undefined) { + clearInterval(ttyStateCheckInterval); + ttyStateCheckInterval = null; + // Retry connection + if (state.activeInst) { + setTimeout(() => connectTTY(), 50); + } + } else if (retries >= maxRetries) { + clearInterval(ttyStateCheckInterval); + ttyStateCheckInterval = null; + console.error('connectTTY: state did not become IDLE after max retries, forcing IDLE'); + ttyState = 'IDLE'; + // Retry connection once after forcing IDLE + if (state.activeInst) { + setTimeout(() => connectTTY(), 50); + } + } + }, 100); + return; + } + + ttyState = 'CONNECTING'; const id = state.activeInst; if (!id) { updateStatus("idle"); return; } const inst = state.instances.find(i => i.id === id); if (inst && inst.status !== 'running') { + ttyState = 'IDLE'; // Reset state before returning updateStatus("stopped"); return; } @@ -1127,16 +1271,17 @@

Start Instance

const ws = new WebSocket(url); ws.binaryType = "arraybuffer"; - let isReady = false; let isFirstData = true; let handshakeTimeout = null; ws.onopen = () => { + ttyState = 'HANDSHAKING'; // Set handshake timeout (5 seconds) handshakeTimeout = setTimeout(() => { - if (!isReady && ws === ttySocket) { + if (ttyState !== 'READY' && ws === ttySocket) { console.error('WebSocket handshake timeout: no "ready" message received'); updateStatus("handshake timeout"); + ttyState = 'DISCONNECTING'; ws.close(); // Fallback to SSE after timeout setTimeout(() => startSSE(), 500); @@ -1145,11 +1290,11 @@

Start Instance

}; ws.onmessage = (ev) => { // Handle handshake: wait for "ready" message first - if (!isReady) { + if (ttyState !== 'READY') { try { const msg = JSON.parse(ev.data); if (msg.type === 'ready') { - isReady = true; + ttyState = 'READY'; // Clear handshake timeout if (handshakeTimeout) { clearTimeout(handshakeTimeout); @@ -1177,7 +1322,7 @@

Start Instance

} else if (term) term.write(ev.data); // After receiving first data (after ready), trigger resize for TUI redraw - if (isFirstData && isReady && term) { + if (isFirstData && ttyState === 'READY' && term) { isFirstData = false; setTimeout(() => { if (term && ws === ttySocket && ws.readyState === WebSocket.OPEN) { @@ -1198,9 +1343,16 @@

Start Instance

clearTimeout(handshakeTimeout); handshakeTimeout = null; } + ttyState = 'IDLE'; if (ttySocket === ws) { - updateStatus("ws closed, retrying..."); - ttyReconnectTimer = setTimeout(connectTTY, 1000); + // Only reconnect if instance is still running + const inst = state.instances.find(i => i.id === state.activeInst); + if (inst && inst.status === 'running') { + updateStatus("ws closed, retrying..."); + ttyReconnectTimer = setTimeout(connectTTY, 1000); + } else { + updateStatus("stopped"); + } } }; ttySocket = ws;