fix: Integrate SafePTYWriter and fix stdin/IPC socket bugs

- Integrated SafePTYWriter and PTYStreamAnalyzer from PR #171 to prevent terminal corruption
- Fixed stdin listener duplication bug - stdin is now handled via IPC socket only
- Fixed IPC socket handler to properly handle both stdin data and control commands using framed message protocol
- Updated socket paths from 'i.sock' to unified 'ipc.sock'
- Added comprehensive tests for SafePTYWriter and PTYStreamAnalyzer
- Removed global process.stdin listeners to prevent duplication across sessions
This commit is contained in:
Peter Steinberger 2025-07-01 12:11:33 +01:00
parent 740aec5ae6
commit 92aac0ccfc
3 changed files with 663 additions and 43 deletions

View file

@ -35,7 +35,13 @@ import { WriteQueue } from '../utils/write-queue.js';
import { AsciinemaWriter } from './asciinema-writer.js';
import { ProcessUtils } from './process-utils.js';
import { SessionManager } from './session-manager.js';
import { frameMessage, MessageType } from './socket-protocol.js';
import {
type ControlCommand,
frameMessage,
MessageParser,
MessageType,
parsePayload,
} from './socket-protocol.js';
import {
type KillControlMessage,
PtyError,
@ -369,12 +375,7 @@ export class PtyManager extends EventEmitter {
// Setup PTY event handlers
this.setupPtyHandlers(session, options.forwardToStdout || false, options.onExit);
// Setup stdin forwarding if forwarding to stdout
if (options.forwardToStdout) {
// Setup stdin forwarding for fwd mode
this.setupStdinForwarding(session);
logger.log(chalk.gray('Stdin forwarding enabled'));
}
// Note: stdin forwarding is now handled via IPC socket
// Setup session.json watcher for title updates (vt title command)
this.setupSessionJsonWatcher(session);
@ -670,23 +671,36 @@ export class PtyManager extends EventEmitter {
logger.debug(`Sent initial ${session.titleMode} title for session ${session.id}`);
}
// Monitor stdin file for input
this.monitorStdinFile(session);
// Setup IPC socket for all communication
this.setupIPCSocket(session);
}
/**
* Monitor stdin file for input data using Unix socket for lowest latency
* Setup Unix socket for all IPC communication
*/
private monitorStdinFile(session: PtySession): void {
private setupIPCSocket(session: PtySession): void {
const ptyProcess = session.ptyProcess;
if (!ptyProcess) {
logger.error(`No PTY process found for session ${session.id}`);
return;
}
// Create Unix domain socket for fast IPC
// Use shorter name to avoid macOS 104 char limit for Unix socket paths
const socketPath = path.join(session.controlDir, 'i.sock');
// Create Unix domain socket for all IPC
// IMPORTANT: macOS has a 104 character limit for Unix socket paths, including null terminator.
// This means the actual usable path length is 103 characters. To avoid EINVAL errors:
// - Use short socket names (e.g., 'ipc.sock' instead of 'vibetunnel-ipc.sock')
// - Keep session directories as short as possible
// - Avoid deeply nested directory structures
const socketPath = path.join(session.controlDir, 'ipc.sock');
// Verify the socket path isn't too long
if (socketPath.length > 103) {
logger.error(`Socket path too long (${socketPath.length} chars): ${socketPath}`);
logger.error(
`macOS limit is 103 characters. Consider using shorter session IDs or control paths.`
);
return;
}
try {
// Remove existing socket if it exists
@ -696,18 +710,22 @@ export class PtyManager extends EventEmitter {
// Socket doesn't exist, this is expected
}
// Create Unix domain socket server
// Create Unix domain socket server with framed message protocol
const inputServer = net.createServer((client) => {
const parser = new MessageParser();
client.setNoDelay(true);
client.on('data', (data) => {
const text = data.toString('utf8');
if (ptyProcess) {
// Write input first for fastest response
ptyProcess.write(text);
// Then record it (non-blocking)
session.asciinemaWriter?.writeInput(text);
client.on('data', (chunk) => {
parser.addData(chunk);
for (const { type, payload } of parser.parseMessages()) {
this.handleSocketMessage(session, type, payload);
}
});
client.on('error', (err) => {
logger.debug(`Client socket error for session ${session.id}:`, err);
});
});
inputServer.listen(socketPath, () => {
@ -726,7 +744,44 @@ export class PtyManager extends EventEmitter {
logger.error(`Failed to create input socket for session ${session.id}:`, error);
}
// Socket-only approach - no FIFO monitoring
// All IPC goes through this socket
}
/**
* Handle incoming socket messages
*/
private handleSocketMessage(session: PtySession, type: MessageType, payload: Buffer): void {
try {
const data = parsePayload(type, payload);
switch (type) {
case MessageType.STDIN_DATA: {
const text = data as string;
if (session.ptyProcess) {
// Write input first for fastest response
session.ptyProcess.write(text);
// Then record it (non-blocking)
session.asciinemaWriter?.writeInput(text);
}
break;
}
case MessageType.CONTROL_CMD: {
const cmd = data as ControlCommand;
this.handleControlMessage(session, cmd);
break;
}
case MessageType.HEARTBEAT:
// Echo heartbeat back (could add heartbeat response later)
break;
default:
logger.debug(`Unknown message type ${type} for session ${session.id}`);
}
} catch (error) {
logger.error(`Failed to handle socket message for session ${session.id}:`, error);
}
}
/**
@ -950,7 +1005,7 @@ export class PtyManager extends EventEmitter {
}
// For forwarded sessions, we need to use socket communication
const socketPath = path.join(sessionPaths.controlDir, 'i.sock');
const socketPath = path.join(sessionPaths.controlDir, 'ipc.sock');
// Check if we have a cached socket connection
let socketClient = this.inputSocketClients.get(sessionId);
@ -978,8 +1033,9 @@ export class PtyManager extends EventEmitter {
}
if (socketClient && !socketClient.destroyed) {
// Write and check for backpressure
const canWrite = socketClient.write(dataToSend);
// Send stdin data using framed message protocol
const message = frameMessage(MessageType.STDIN_DATA, dataToSend);
const canWrite = socketClient.write(message);
if (!canWrite) {
// Socket buffer is full
logger.debug(`Socket buffer full for session ${sessionId}, data queued`);
@ -1703,17 +1759,12 @@ export class PtyManager extends EventEmitter {
private setupStdinForwarding(session: PtySession): void {
if (!session.ptyProcess) return;
// Create and store the listener to enable cleanup
const stdinDataListener = (data: Buffer) => {
try {
session.ptyProcess?.write(data.toString());
} catch (error) {
logger.error(`Failed to forward stdin to session ${session.id}:`, error);
}
};
session.stdinDataListener = stdinDataListener;
process.stdin.on('data', stdinDataListener);
// IMPORTANT: stdin forwarding is now handled via IPC socket in fwd.ts
// This method is kept for backward compatibility but should not be used
// as it would cause stdin duplication if multiple sessions are created
logger.warn(
`setupStdinForwarding called for session ${session.id} - stdin should be handled via IPC socket`
);
}
/**
@ -1742,7 +1793,7 @@ export class PtyManager extends EventEmitter {
// Unref the server so it doesn't keep the process alive
session.inputSocketServer.unref();
try {
fs.unlinkSync(path.join(session.controlDir, 'i.sock'));
fs.unlinkSync(path.join(session.controlDir, 'ipc.sock'));
} catch (_e) {
// Socket already removed
}
@ -1757,10 +1808,6 @@ export class PtyManager extends EventEmitter {
session.sessionJsonWatcher.close();
}
// Clean up stdin listener
if (session.stdinDataListener) {
process.stdin.removeListener('data', session.stdinDataListener);
session.stdinDataListener = undefined;
}
// Note: stdin handling is now done via IPC socket, no global listeners to clean up
}
}

View file

@ -0,0 +1,299 @@
import type { IPty } from 'node-pty';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { SafePTYWriter } from '../../server/pty/safe-pty-writer.js';
describe('SafePTYWriter', () => {
let mockPty: IPty;
let onDataCallback: ((data: string) => void) | undefined;
let writer: SafePTYWriter;
beforeEach(() => {
vi.useFakeTimers();
// Create mock PTY
mockPty = {
onData: vi.fn((callback: (data: string) => void) => {
onDataCallback = callback;
}),
write: vi.fn(),
resize: vi.fn(),
kill: vi.fn(),
pid: 12345,
process: 'test',
handleFlowControl: false,
onExit: vi.fn(),
pause: vi.fn(),
resume: vi.fn(),
clear: vi.fn(),
} as unknown as IPty;
writer = new SafePTYWriter(mockPty, { debug: false });
});
afterEach(() => {
vi.clearAllTimers();
vi.restoreAllMocks();
});
describe('basic functionality', () => {
it('should attach to PTY and intercept data', () => {
const onData = vi.fn();
writer.attach(onData);
expect(mockPty.onData).toHaveBeenCalledWith(expect.any(Function));
// Simulate PTY output
onDataCallback?.('Hello World');
expect(onData).toHaveBeenCalledWith('Hello World');
});
it('should queue titles for injection', () => {
writer.queueTitle('Test Title');
expect(writer.getPendingCount()).toBe(1);
// New title replaces the previous one
writer.queueTitle('Another Title');
expect(writer.getPendingCount()).toBe(1);
});
it('should clear pending titles', () => {
writer.queueTitle('Test Title');
// New title replaces the previous one
writer.queueTitle('Another Title');
expect(writer.getPendingCount()).toBe(1);
writer.clearPending();
expect(writer.getPendingCount()).toBe(0);
});
});
describe('safe injection at newlines', () => {
it('should inject title after newline', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Safe Title');
// Send data with newline
onDataCallback?.('Hello World\n');
expect(onData).toHaveBeenCalledWith('Hello World\n\x1b]0;Safe Title\x07');
expect(writer.getPendingCount()).toBe(0);
});
it('should only inject latest title when multiple are queued', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Title 1');
writer.queueTitle('Title 2'); // This replaces Title 1
// Send data with newline
onDataCallback?.('Line 1\nLine 2\n');
// Should only inject the latest title (Title 2)
expect(onData).toHaveBeenCalledWith('Line 1\n\x1b]0;Title 2\x07Line 2\n');
expect(writer.getPendingCount()).toBe(0);
});
});
describe('safe injection at carriage returns', () => {
it('should inject title after carriage return', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('CR Title');
// Send data with carriage return
onDataCallback?.('Progress: 100%\r');
expect(onData).toHaveBeenCalledWith('Progress: 100%\r\x1b]0;CR Title\x07');
expect(writer.getPendingCount()).toBe(0);
});
});
describe('safe injection after escape sequences', () => {
it('should inject after CSI sequence', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('After CSI');
// Send data with color escape sequence
onDataCallback?.('\x1b[31mRed Text\x1b[0m');
// Should inject after either escape sequence (both are safe)
const callArg = onData.mock.calls[0][0];
// biome-ignore lint/suspicious/noControlCharactersInRegex: Testing ANSI escape sequences
expect(callArg).toMatch(/\x1b\[31m.*\x1b\]0;After CSI\x07.*Red Text\x1b\[0m/);
expect(writer.getPendingCount()).toBe(0);
});
it('should not inject in middle of escape sequence', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Mid Escape');
// Send incomplete escape sequence
onDataCallback?.('\x1b[31');
// Should not inject yet
expect(onData).toHaveBeenCalledWith('\x1b[31');
expect(writer.getPendingCount()).toBe(1); // Still pending
// Complete the sequence
onDataCallback?.('m');
// Now it should inject
expect(onData).toHaveBeenCalledWith('m\x1b]0;Mid Escape\x07');
expect(writer.getPendingCount()).toBe(0);
});
});
describe('safe injection at prompt patterns', () => {
it('should inject after bash prompt', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Prompt Title');
// Send data with bash prompt
onDataCallback?.('user@host:~$ ');
expect(onData).toHaveBeenCalledWith('user@host:~$ \x1b]0;Prompt Title\x07');
expect(writer.getPendingCount()).toBe(0);
});
it('should inject after root prompt', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Root Title');
// Send data with root prompt
onDataCallback?.('root@host:/# ');
expect(onData).toHaveBeenCalledWith('root@host:/# \x1b]0;Root Title\x07');
expect(writer.getPendingCount()).toBe(0);
});
});
describe('idle injection', () => {
it('should inject during idle period', () => {
const writer = new SafePTYWriter(mockPty, { idleThreshold: 50 });
const onData = vi.fn();
writer.attach(onData);
// Queue title but send data without safe points
writer.queueTitle('Idle Title');
onDataCallback?.('Some output');
// Title should not be injected yet
expect(writer.getPendingCount()).toBe(1);
expect(mockPty.write).not.toHaveBeenCalled();
// Advance time past idle threshold
vi.advanceTimersByTime(60);
// Title should be injected directly to PTY
expect(mockPty.write).toHaveBeenCalledWith('\x1b]0;Idle Title\x07');
expect(writer.getPendingCount()).toBe(0);
});
it('should reset idle timer on new output', () => {
const writer = new SafePTYWriter(mockPty, { idleThreshold: 50 });
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Reset Timer');
onDataCallback?.('Output 1');
// Advance time partially
vi.advanceTimersByTime(30);
// New output should reset timer
onDataCallback?.('Output 2');
// Advance time past original threshold
vi.advanceTimersByTime(30);
// Should not have injected yet (timer was reset)
expect(mockPty.write).not.toHaveBeenCalled();
// Advance remaining time
vi.advanceTimersByTime(30);
// Now it should inject
expect(mockPty.write).toHaveBeenCalledWith('\x1b]0;Reset Timer\x07');
});
});
describe('UTF-8 safety', () => {
it('should not inject in middle of UTF-8 sequence', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('UTF8 Title');
// Send partial UTF-8 sequence for emoji 😀 (F0 9F 98 80)
onDataCallback?.('Hello \xF0\x9F');
// Should not inject in middle of UTF-8
expect(onData).toHaveBeenCalledWith('Hello \xF0\x9F');
expect(writer.getPendingCount()).toBe(1);
// Complete UTF-8 sequence and add newline
onDataCallback?.('\x98\x80\n');
// Should inject after newline
expect(onData).toHaveBeenCalledWith('\x98\x80\n\x1b]0;UTF8 Title\x07');
expect(writer.getPendingCount()).toBe(0);
});
});
describe('force injection', () => {
it('should force inject pending title', () => {
writer.queueTitle('Force 1');
writer.queueTitle('Force 2'); // This replaces Force 1
expect(writer.getPendingCount()).toBe(1);
writer.forceInject();
expect(mockPty.write).toHaveBeenCalledWith('\x1b]0;Force 2\x07');
expect(writer.getPendingCount()).toBe(0);
});
});
describe('edge cases', () => {
it('should handle empty data', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Empty Data');
onDataCallback?.('');
expect(onData).toHaveBeenCalledWith('');
expect(writer.getPendingCount()).toBe(1);
});
it('should handle detach', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Detach Test');
writer.detach();
// Should clear pending titles
expect(writer.getPendingCount()).toBe(0);
});
it('should handle multiple safe points in single chunk', () => {
const onData = vi.fn();
writer.attach(onData);
writer.queueTitle('Multi 1');
writer.queueTitle('Multi 2');
writer.queueTitle('Multi 3'); // Only this one will be injected
// Send data with multiple safe points
onDataCallback?.('Line 1\nLine 2\nLine 3\n');
// Only latest title should be injected at first safe point
expect(onData).toHaveBeenCalledWith('Line 1\n\x1b]0;Multi 3\x07Line 2\nLine 3\n');
expect(writer.getPendingCount()).toBe(0);
});
});
});

View file

@ -0,0 +1,274 @@
import { beforeEach, describe, expect, it } from 'vitest';
import { PTYStreamAnalyzer } from '../../server/pty/stream-analyzer.js';
describe('PTYStreamAnalyzer', () => {
let analyzer: PTYStreamAnalyzer;
beforeEach(() => {
analyzer = new PTYStreamAnalyzer();
});
describe('newline detection', () => {
it('should detect newline as safe injection point', () => {
const buffer = Buffer.from('Hello World\n');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0]).toEqual({
position: 12, // After newline
reason: 'newline',
confidence: 100,
});
});
it('should detect multiple newlines', () => {
const buffer = Buffer.from('Line 1\nLine 2\nLine 3\n');
const points = analyzer.process(buffer);
expect(points).toHaveLength(3);
expect(points.map((p) => p.position)).toEqual([7, 14, 21]);
expect(points.every((p) => p.reason === 'newline')).toBe(true);
});
});
describe('carriage return detection', () => {
it('should detect carriage return as safe injection point', () => {
const buffer = Buffer.from('Progress: 100%\r');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0]).toEqual({
position: 15, // After \r
reason: 'carriage_return',
confidence: 90,
});
});
it('should handle CRLF', () => {
const buffer = Buffer.from('Windows line\r\n');
const points = analyzer.process(buffer);
expect(points).toHaveLength(2);
expect(points[0].reason).toBe('carriage_return');
expect(points[1].reason).toBe('newline');
});
});
describe('ANSI escape sequence detection', () => {
it('should detect end of CSI color sequence', () => {
const buffer = Buffer.from('\x1b[31mRed\x1b[0m');
const points = analyzer.process(buffer);
expect(points).toHaveLength(2);
expect(points[0]).toEqual({
position: 5, // After \x1b[31m
reason: 'sequence_end',
confidence: 80,
});
expect(points[1]).toEqual({
position: 12, // After \x1b[0m
reason: 'sequence_end',
confidence: 80,
});
});
it('should handle cursor movement sequences', () => {
const buffer = Buffer.from('\x1b[2A\x1b[3B');
const points = analyzer.process(buffer);
expect(points).toHaveLength(2);
expect(points.every((p) => p.reason === 'sequence_end')).toBe(true);
});
it('should handle complex CSI sequences', () => {
const buffer = Buffer.from('\x1b[38;5;196m'); // 256 color
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0].position).toBe(11);
});
it('should not inject in middle of escape sequence', () => {
const buffer1 = Buffer.from('\x1b[31');
const points1 = analyzer.process(buffer1);
expect(points1).toHaveLength(0);
const buffer2 = Buffer.from('m');
const points2 = analyzer.process(buffer2);
expect(points2).toHaveLength(1);
expect(points2[0].position).toBe(1);
});
});
describe('OSC sequence detection', () => {
it('should detect end of OSC title sequence', () => {
const buffer = Buffer.from('\x1b]0;Terminal Title\x07');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0]).toEqual({
position: 19, // After BEL
reason: 'sequence_end',
confidence: 80,
});
});
it('should handle OSC with ST terminator', () => {
const buffer = Buffer.from('\x1b]0;Title\x1b\\');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0].position).toBe(11); // After ESC\
});
});
describe('prompt pattern detection', () => {
it('should detect bash prompt', () => {
const buffer = Buffer.from('user@host:~$ ');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0]).toEqual({
position: 13,
reason: 'prompt',
confidence: 85,
});
});
it('should detect root prompt', () => {
const buffer = Buffer.from('root@server:/# ');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0].reason).toBe('prompt');
});
it('should detect fish prompt', () => {
const buffer = Buffer.from('~/projects> ');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0].reason).toBe('prompt');
});
it('should detect modern prompt', () => {
const buffer = Buffer.from('~/code ');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0].reason).toBe('prompt');
});
it('should detect Python REPL prompt', () => {
const _buffer = Buffer.from('>>> ');
analyzer.process(Buffer.from('>>'));
const points = analyzer.process(Buffer.from('> '));
expect(points).toHaveLength(1);
expect(points[0].reason).toBe('prompt');
});
});
describe('UTF-8 handling', () => {
it('should not inject in middle of 2-byte UTF-8', () => {
// € symbol: C2 A2
const buffer1 = Buffer.from([0xc2]);
const points1 = analyzer.process(buffer1);
expect(points1).toHaveLength(0);
const buffer2 = Buffer.from([0xa2, 0x0a]); // Complete char + newline
const points2 = analyzer.process(buffer2);
expect(points2).toHaveLength(1);
expect(points2[0].position).toBe(2); // After newline
});
it('should not inject in middle of 3-byte UTF-8', () => {
// ∑ symbol: E2 88 91
const buffer = Buffer.from([0xe2, 0x88]);
const points = analyzer.process(buffer);
expect(points).toHaveLength(0);
});
it('should not inject in middle of 4-byte UTF-8', () => {
// 😀 emoji: F0 9F 98 80
const buffer = Buffer.from([0xf0, 0x9f, 0x98]);
const points = analyzer.process(buffer);
expect(points).toHaveLength(0);
});
it('should handle complete UTF-8 sequences', () => {
const buffer = Buffer.from('Hello 世界\n');
const points = analyzer.process(buffer);
expect(points).toHaveLength(1);
expect(points[0].reason).toBe('newline');
});
});
describe('state management', () => {
it('should maintain state across multiple process calls', () => {
// Split escape sequence across buffers
const buffer1 = Buffer.from('\x1b[');
const points1 = analyzer.process(buffer1);
expect(points1).toHaveLength(0);
const buffer2 = Buffer.from('31m');
const points2 = analyzer.process(buffer2);
expect(points2).toHaveLength(1);
});
it('should reset state correctly', () => {
// Process partial sequence
analyzer.process(Buffer.from('\x1b[31'));
// Reset
analyzer.reset();
// Should treat new ESC as start of sequence
const points = analyzer.process(Buffer.from('\x1b[0m'));
expect(points).toHaveLength(1);
});
it('should provide state information', () => {
const state1 = analyzer.getState();
expect(state1.inEscape).toBe(false);
analyzer.process(Buffer.from('\x1b['));
const state2 = analyzer.getState();
expect(state2.inEscape).toBe(true);
});
});
describe('complex scenarios', () => {
it('should handle mixed content', () => {
const buffer = Buffer.from('Normal text\n\x1b[32mGreen\x1b[0m\rProgress\n$ ');
const points = analyzer.process(buffer);
expect(points.length).toBeGreaterThan(0);
const reasons = points.map((p) => p.reason);
expect(reasons).toContain('newline');
expect(reasons).toContain('sequence_end');
expect(reasons).toContain('carriage_return');
expect(reasons).toContain('prompt');
});
it('should handle rapid color changes', () => {
const buffer = Buffer.from('\x1b[31mR\x1b[32mG\x1b[34mB\x1b[0m');
const points = analyzer.process(buffer);
expect(points).toHaveLength(4); // After each sequence
expect(points.every((p) => p.reason === 'sequence_end')).toBe(true);
});
it('should handle real terminal output', () => {
// Simulating 'ls --color' output
const buffer = Buffer.from(
'\x1b[0m\x1b[01;34mdir1\x1b[0m\n' + '\x1b[01;32mexecutable\x1b[0m\n' + 'file.txt\n'
);
const points = analyzer.process(buffer);
const newlines = points.filter((p) => p.reason === 'newline');
expect(newlines).toHaveLength(3);
});
});
});