Feat/ts pty handler migration by kartikloops · Pull Request #126 · The-OpenROAD-Project/OpenROAD-MCP

kartikloops · 2026-06-12T03:52:08Z

Summary

Migrates the interactive PTY layer from Python to TypeScript — source and tests.

New source files

src/interactive/models.ts — session error hierarchy (SessionError, SessionTerminatedError, CommandBlockedError, PTYError)
src/interactive/buffer.ts — string-based circular buffer with mutex and async wait-for-data
src/interactive/pty_handler.ts — wraps node-pty, handles spawn, write, graceful termination, and event-driven output via onData/onExit
src/interactive/session.ts — session lifecycle (CREATING → ACTIVE → TERMINATED), writer task, input queue, output collection
src/core/models.ts — SessionState enum and result interfaces

Tests

buffer.test.ts, pty_handler.test.ts, command_validation.test.ts, session.test.ts
222 tests, all passing

kartikloops · 2026-06-12T03:54:19Z

@luarss Pleas have a review when you get sometime

Copilot

Pull request overview

Migrates the interactive PTY/session layer from Python to a new TypeScript implementation, introducing a TS-based PTY wrapper, buffered output collection, session lifecycle management, and accompanying Vitest coverage.

Changes:

Added a new interactive subsystem (PtyHandler, InteractiveSession, CircularBuffer) and error model hierarchy in TypeScript.
Introduced SessionState plus interactive result/info interfaces in src/core/models.ts.
Added comprehensive Vitest coverage for the new interactive components and updated ESLint config to better support the new code/tests.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
typescript/src/interactive/session.ts	New `InteractiveSession` lifecycle, input queue writer loop, output collection + error detection.
typescript/src/interactive/pty_handler.ts	New `node-pty` wrapper (spawn, write, exit handling, termination, cleanup, command validation).
typescript/src/interactive/models.ts	New interactive error hierarchy (`SessionError`, `PTYError`, etc.).
typescript/src/interactive/buffer.ts	New mutex-protected string chunk circular buffer with async wait-for-data.
typescript/src/core/models.ts	New `SessionState` enum and interactive result/info interfaces.
typescript/eslint.config.ts	Ignore `coverage/**` and enforce `@typescript-eslint/no-unused-vars` (with `_` ignore patterns); relax return-type rule for tests.
typescript/tests/interactive/session.test.ts	Tests for session lifecycle, writer loop behavior, output reads, and error detection.
typescript/tests/interactive/pty_handler.test.ts	Tests for PTY spawn wiring, onData/onExit behavior, termination, waitForExit, and cleanup semantics.
typescript/tests/interactive/command_validation.test.ts	Tests for allowlist + metacharacter/redirection blocking and Settings env parsing.
typescript/tests/interactive/buffer.test.ts	Tests for buffer append/drain/eviction, waitForData, stats, and concurrency behavior.
.vscode/settings.json	Workspace TS SDK path for consistent editor TypeScript usage.
plan.md	Present in PR metadata (currently empty).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

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>

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

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>

luarss

Posted some high priorities one to solve, please check @kartikloops

luarss · 2026-06-13T11:00:33Z

@@ -0,0 +1,3 @@
+{


why is this needed?

Without it, every contributor who opens the repo in VS Code gets TypeScript errors or the wrong type-checker, because VS Code defaults to its own bundled TypeScript version rather than the project's. This file pins VS Code to use the TypeScript installed in typescript/node_modules/, so everyone gets consistent IntelliSense and error highlighting that matches what tsc actually sees.

It ensures VS Code uses the same TypeScript the build does.

… indefinitely

…clusive

…mutex

…minated

…ction

luarss · 2026-06-14T09:45:46Z

        logger.debug("PTY handler cleanup completed")
+
+
+class MacOSPTYHandler(PTYHandler):


Lets avoid this for now - don't need to support macOS for a quick PoC.

luarss · 2026-06-14T09:51:16Z

+    const exited = await this.waitForExit(5000);
+    if (exited === null && this._alive) {
+      try {
+        this._ptyProcess.kill("SIGKILL");


terminateProcess() sends SIGKILL and returns immediately without awaiting; cleanup() then disposes the exit listener before SIGKILL is acknowledged, permanently losing the real exit code.

When the process survives the 5-second SIGTERM window: SIGKILL is sent at line 162 and terminateProcess() returns without awaiting. cleanup() then calls this._exitDisposable?.dispose() (line 179), removing the onExit listener. When SIGKILL is eventually processed, the listener is gone — _exitCode stays null forever. Any pending waitForExit() callers receive null instead of the real exit code (e.g. 137).

luarss · 2026-06-14T09:52:13Z

+          `Command argument ${i} contains shell metacharacters which are not allowed: ${JSON.stringify(arg)}`,
+        );
+      }
+      if (arg.startsWith(">") || arg.startsWith("<")) {


validateCommand does not check for path traversal in arguments — openroad -script ../../etc/malicious.tcl passes all validation.

The argument loop (lines 43–55) checks for shell metacharacters and leading >/<, but never inspects arguments for .. sequences. ['openroad', '-script', '../../etc/malicious.tcl'] contains no flagged characters, so it passes all checks and OpenROAD executes the attacker-controlled Tcl script.

luarss · 2026-06-14T09:52:33Z

+      this._chunks.push(data);
+      this._totalSize += data.length;
+
+      while (this._totalSize > this.maxSize && this._chunks.length > 1) {


A single append larger than maxSize is never evicted — the buffer permanently exceeds its capacity.

The eviction loop condition is this._chunks.length > 1. When a single large chunk is the only entry, _chunks.length === 1 and the loop never runs, leaving _totalSize above maxSize indefinitely.

luarss · 2026-06-14T09:53:49Z

+
+    const executable = command[0]!;
+
+    if (path.isAbsolute(executable)) {


Python implementation accepts absolute path though. Why is there a divergence ?

kartikloops added 12 commits June 11, 2026 21:47

add vscode workspace settings for typescript sdk path

7c1185e

add plan doc

aae2f60

add core session state and error models

40517ea

add interactive session error models

e222bbc

add circular buffer for pty output

17fb53e

add pty handler for managing node-pty processes

64542ed

add interactive session manager

11780e7

add tests for circular buffer

0540afa

add tests for command validation

1fd693b

add tests for pty handler

b182b9b

add tests for interactive session

907bea4

update eslint config with coverage ignore and stricter rules

b9cd166

Copilot AI review requested due to automatic review settings June 12, 2026 03:52

kartikloops changed the base branch from main to feat/ts-migration June 12, 2026 03:52

Copilot started reviewing on behalf of kartikloops June 12, 2026 03:52 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

kartikloops added 5 commits June 11, 2026 22:02

fix pending waiters, env cast and resolver types in pty handler

edafb12

guard read chunk size against zero in session onData handler

fed1d4e

add test for cleanup resolving pending waitForExit waiters

15871aa

add real openroad repl integration check script

40f64c8

code cleanup

e88e49e

kartikloops requested a review from Copilot June 12, 2026 04:04

Copilot started reviewing on behalf of kartikloops June 12, 2026 04:04 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread typescript/src/interactive/buffer.ts

Comment thread typescript/src/interactive/pty_handler.ts

Potential fix for pull request finding

dd62da5

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>

kartikloops requested a review from Copilot June 12, 2026 04:09

Copilot started reviewing on behalf of kartikloops June 12, 2026 04:10 View session

kartikloops self-assigned this Jun 12, 2026

kartikloops requested a review from luarss June 12, 2026 04:11

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread typescript/src/interactive/session.ts Outdated

Comment thread typescript/src/interactive/session.ts

Comment thread typescript/scripts/integration_check.ts

Comment thread typescript/scripts/integration_check.ts

Comment thread typescript/scripts/integration_check.ts

added case for READ_CHUNK_SIZE is larger than limit

0f64b95

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>

luarss requested changes Jun 13, 2026

View reviewed changes

kartikloops added 17 commits June 13, 2026 10:26

fix isAlive signaling shutdown when process death is detected

c9b578e

add test for isAlive shutdown signal on process death

f5e3e22

fix force terminate returning before alive flag clears

5e41bea

update tests for force terminate waiting for exit

9f6862e

fix terminate not disposing pty listeners and pending waiters

d9ffce4

update terminate tests to assert pty cleanup is called

e9bd5cf

fix clear not waking pending waitForData callers

7f6969d

add regression test for clear waking pending waitForData callers

f86aecd

signal shutdown in readOutput drain path so writer task does not loop…

68f96c7

… indefinitely

add regression test for readOutput signaling shutdown on dead session

527b225

fix waitForData not re-checking dataAvailable under mutex after runEx…

2856095

…clusive

add regression test for waitForData race between fast-path check and …

ac2735d

…mutex

handle append rejection in onData handler to avoid silent data loss

164ff9b

add regression test for append rejection transitioning session to ter…

cfc55d3

…minated

move MacOSPTYHandler to production and add create_pty_handler factory

a7615f3

use create_pty_handler so macOS gets the keepalive and drain subclass

77dc249

remove duplicate MacOSPTYHandler from test file and import from produ…

d180b0f

…ction

kartikloops requested a review from luarss June 13, 2026 16:34

luarss requested changes Jun 14, 2026

View reviewed changes

		logger.debug("PTY handler cleanup completed")


		class MacOSPTYHandler(PTYHandler):


		const executable = command[0]!;

		if (path.isAbsolute(executable)) {

Conversation

kartikloops commented Jun 12, 2026

Summary

New source files

Tests

Uh oh!

kartikloops commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luarss left a comment

Choose a reason for hiding this comment

Uh oh!

luarss Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

kartikloops Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luarss Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

luarss Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

luarss Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

luarss Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

luarss Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants