Add Windows PTY support for the Python worker#73
Draft
t-kalinowski wants to merge 33 commits into
Draft
Conversation
t-kalinowski
force-pushed
the
worker-protocol-windows
branch
2 times, most recently
from
130afff to
65f03fe
Compare
Run built-in Python through ConPTY on Windows so it uses the same PTY-backed stdin and sideband accounting path as Unix. Keep the worker protocol unchanged while adding Windows console setup, input draining, and zod/Python parity coverage. Verification: cargo check; cargo build; python tests/run_integration_tests.py --binary target/debug/mcp-repl.exe; cargo clippy --all-targets --all-features -- -D warnings; cargo test --quiet; cargo +nightly fmt. The required python3 integration command could not run because python3 is not installed on this Windows host.
Review finding: The Windows Python backend now emits raw ConPTY cursor-control sequences in normal replies, breaking an existing Python backend test and polluting user-visible output. Review comment: - [P1] Strip ConPTY cursor-control bytes from Python output C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:7337-7339 When the Windows PTY path is used, this raw reader forwards VT control sequences emitted by ConPTY (e.g. \u001b[?25l\u001b[15;1H\u001b[?25h) directly into MCP replies; a simple Python reply now includes these bytes and python_backend_runs_inside_mcp_repl_worker fails on Windows. Please filter/suppress the ConPTY screen-control traffic before appending stdout, otherwise Windows Python sessions leak terminal escape codes to clients. Response: Added a Windows PTY output filter that strips ConPTY screen-control CSI sequences before append_raw_text, including split sequences, while preserving SGR sequences. Added regression assertions in python_backend_runs_inside_mcp_repl_worker and unit coverage for split ConPTY cursor sequences.
t-kalinowski
force-pushed
the
worker-protocol-windows
branch
from
65f03fe to
fde100d
Compare
Review finding:
[P1] Account direct stdin reads on the Windows PTY path ? C:\Users\kalin\Documents\GitHub\mcp-repl\src\backend.rs:42-45
On Windows Python now takes the PTY path, but `sys.stdin`/`os.read(0, ...)` remain CPython's direct console readers while only the PyOS_Readline hook emits `readline_input`. If code consumes buffered input via `sys.stdin.readline()` followed by a data line in the same tool call, those bytes are removed from the PTY without clearing `active_stdin`, so the next prompt/code line hits a `readline_input text does not match active stdin` protocol error and resets the worker. Either keep sideband-aware stdin wrappers for Windows or account direct fd reads before defaulting to PTY.
Response:
Installed Windows PTY stdin bridges for `sys.stdin`, fd-backed open/fdopen/FileIO, `os.read`, and `nt.read`, and added Windows regression coverage for buffered `sys.stdin.readline()` plus `os.read(0, ...)` followed by more REPL input.
Review finding:
[P1] Normalize CRLF before Windows PTY stdin accounting ? C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:7904-7908
For Windows PTY input, this translation changes only `\n` to `\r` while the server still tracks the original payload in `active_stdin`. When a client sends CRLF input such as `print('A')\r\nprint('B')`, the console/readline sideband is LF-normalized, so `account_active_stdin` compares it against raw `\r\n` and reports a protocol mismatch. Normalize the bytes used for stdin accounting, or translate CRLF as a single terminal newline, before writing to the PTY.
Response:
Normalized Windows Python PTY request text before payload accounting, translated CRLF/CR/LF to a single PTY Enter, consumed ConPTY CRLF pairs at C stdio, and covered CRLF Python input with a Windows regression test.
Review finding:
[P2] Strip OSC sequences from Windows PTY output ? C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:7357-7362
This escape-state branch passes every non-CSI escape sequence through unchanged, so ConPTY title updates like `ESC]0;...BEL` leak into normal Windows Python replies. A simple `1+1` response is prefixed with the raw title-control bytes before the result, which MCP clients will display as junk/control text. Add OSC/string-control handling to the filter or strip ConPTY's title sequence before appending output.
Response:
Extended the Windows PTY output filter to consume OSC and ANSI string-control sequences through BEL or ST terminators, with split-sequence regression coverage.
Review finding:
[P2] Honor fd 0 replacement before bridging Windows reads ? C:\Users\kalin\Documents\GitHub\mcp-repl\python\embedded.py:800-801
On Windows this returns `True` for descriptor 0 without verifying that it still refers to the original managed stdin. If user code closes or replaces fd 0, e.g. with `os.dup2(file_fd, 0)`, subsequent `os.read(0, ...)` is incorrectly routed back through the MCP stdin bridge instead of reading the new file/pipe, which can consume remaining tool input and trigger active-stdin protocol errors. The identity check below should still be used for fd 0 after it may have been replaced.
Response:
Removed the unconditional Windows fd-0 bridge shortcut so fd 0 uses the recorded stdin identity check, and added a Windows regression test that replaces fd 0 with a file, reads it, restores stdin, and continues REPL input.
Review finding:
[P2] Preserve buffered input() prompts on Windows PTY ? C:\Users\kalin\Documents\GitHub\mcp-repl\python\embedded.py:1073-1077
In this Windows PTY branch `sys.stdin` becomes an `McpInputStream`, but `builtins.input` is left as CPython's implementation, so `input("p> ")` writes the prompt separately and then calls `sys.stdin.readline()` without passing the prompt into the bridge. When the answer is already buffered in the same tool call, e.g. `print('got', input('p> '))\nhello`, the visible reply omits `p> `, regressing the non-PTY path that installed `_input`. Route Windows PTY `input()` through the same prompt-aware wrapper or otherwise pass the prompt to the bridge.
Response:
Installed the prompt-aware `_input` wrapper on the Windows PTY branch and made the buffered input prompt assertion apply on Windows.
Review finding:
[P1] Avoid PTY-wrapping the Windows sandbox launcher — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:6163-6165
When Windows sandboxing is enabled, `prepared.program` is the `mcp-repl --windows-sandbox` wrapper, not the embedded Python worker. Attaching the ConPTY here puts the wrapper on the terminal while the actual Python worker still receives pipe stdio from the wrapper, so the new PTY-backed Python protocol loses its TTY assumptions; a basic `--interpreter python --sandbox read-only` request echoes the input and times out waiting for the final prompt. Please attach the PTY to the sandboxed child or fall back to the pipe/legacy driver for sandboxed Windows Python.
Response:
Made the effective stdin transport depend on the resolved sandbox state, falling back to pipe plus the legacy Python driver for sandboxed Windows Python while preserving PTY for unsandboxed Windows and Unix Python. Added unit coverage for the effective transport and a Windows read-only Python sandbox smoke test.
Review finding:
[P2] Filter ConPTY's initial SGR reset — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:7434-7434
On a fresh Windows PTY session, ConPTY emits a leading SGR reset (`\x1b[m`) before the first worker stdout. Because SGR (`m`) falls through to `_ => false`, `WindowsPtyOutputFilter` preserves it, so the first simple Python reply starts with that escape sequence (`print('x')` returns `\x1b[mx\n>>> `). This should be filtered as a ConPTY artifact or simple output contains terminal controls.
Response:
Filtered only leading SGR reset CSI sequences before visible output while preserving normal SGR styling later in the stream, with regression coverage for the initial reset case.
Finding:
[P1] Use protocol accounting for Windows pipe fallback — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:1320-1322
When built-in Python falls back to pipe transport on Windows sandboxed sessions, this branch keeps using `PythonBackendDriver`, which calls `begin_request()` instead of tracking active stdin, while the worker now emits Windows `readline_input` events. In `--interpreter python --sandbox read-only`, `print(input())\nhello` fails with `readline_input reported input with no active turn`, and multi-line requests like `print('A')\nprint('B')` execute but never complete and time out. The pipe fallback needs to use compatible stdin accounting or keep the old non-PTY completion path.
Response:
The Windows sandbox pipe fallback now keeps the old non-PTY completion path instead of emitting protocol `readline_input` for bytes CPython consumed through C `FILE*` before the worker can observe them. The worker still tracks delivered pipe bytes locally so multiline requests and same-call `input()` answers complete, and the server starts its legacy request boundary only after the worker accepts the write.
Finding:
[P1] Preserve Unicode in Windows PTY input — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:8012-8014
For unsandboxed Windows Python sessions, this PTY path passes every non-newline byte through unchanged, but non-ASCII REPL input is delivered to the child as `?`; for example `print('é')` prints `?`, and `input()` of `é` returns `?`. This regresses the default Windows Python backend for any Unicode source or stdin data, so the PTY writer needs a Unicode-safe way to feed ConPTY instead of forwarding raw UTF-8 bytes here.
Response:
The Windows PTY/console path now reads stdin through `ReadConsoleW` and converts UTF-16 console input back to UTF-8 bytes before Python accounting and execution, preserving Unicode source and prompted input while keeping protocol accounting on the console-backed PTY path.
Finding: [P2] Preserve CRLF state across Windows raw reads — C:/Users/kalin/Documents/GitHub/mcp-repl/src/python_session.rs:2030-2032 When Windows PTY-backed Python code uses small raw reads such as `os.read(0, 1)`, `ReadFile` can return the console's `\r\n` line ending split across calls. Because each chunk is normalized independently here, one submitted line like `ab\n` is exposed as `b'a'`, `b'b'`, `b'\n'`, `b'\n'`, leaving an extra blank line for the REPL. The raw-read path needs to carry pending-CR state or otherwise coalesce split CRLF before returning/accounting bytes. Response: Windows raw stdin now normalizes console CRLF with a pending-LF state shared with the later console line-read path. A CR at the end of one raw chunk returns one `\n`, and a following LF is dropped before it can become an extra raw byte or an extra REPL blank line. Added a Windows `os.read(0, 1)` regression that verifies the split line is reported as `b'a'`, `b'b'`, `b'\n'` and the following REPL input still runs. Finding: [P2] Preserve PATH lookup for Windows PTY workers — C:/Users/kalin/Documents/GitHub/mcp-repl/src/worker_process.rs:7821-7823 For Windows PTY custom workers, passing `program.as_mut_ptr()` as `lpApplicationName` disables the normal PATH search that `Command::spawn` provides for pipe workers. A worker spec such as `"executable": "zod-worker.exe"` with its directory on PATH works with `stdin: "pipe"` but fails under `stdin: "pty"` with `os error 2`. Resolve the executable first or pass a null application name and keep the quoted command line. Response: The Windows PTY launcher now passes a null `lpApplicationName` to `CreateProcessW` while keeping the mutable quoted command line, restoring Windows executable lookup behavior. Added a Windows Zod PTY regression that launches `zod-worker.exe` by name from PATH.
Finding: Windows PTY Python interrupt never sends an OS control This new Windows PTY-backed Python path sends the private python_interrupt cleanup message and waits for the ack, but then calls process.send_interrupt(), whose Windows implementation is currently a no-op. That means Ctrl-C only works when the Python worker cooperates with the sideband message; the server never delivers the OS-level interrupt promised by the worker protocol. Please make the ConPTY child interruptible from the server, for example by spawning it in a Windows process group and using GenerateConsoleCtrlEvent or the appropriate ConPTY control path here. Response: Windows generic interrupt routing now sends Ctrl-Break to the worker process group. ConPTY launches include CREATE_NEW_PROCESS_GROUP so the PTY child can be targeted, and Windows unit tests cover both the creation flag and Ctrl-Break dispatch.
Finding: [P2] Drop stale stdin-write acks before waiting — src/worker_process.rs:923 On the pipe-backed Python driver, this waits for `stdin_write_ack` before `driver_on_input_start()` resets the inbox. If a previous timed-out request left a late ack queued, `wait_for_stdin_write_ack` consumes that stale ack and the server proceeds to write the new payload before the worker has installed the new active request, which can make sandboxed Windows Python lose input accounting or time out. Reset/drop stale acks before waiting, as the previous ordering did. Response: Moved the Python pipe driver request reset before the stdin write announcement and added a stale-ack regression that verifies an old ack is dropped before waiting for the new worker ack. Finding: [P2] Count raw stdin consumption by newlines — src/python_session.rs:2205 For sandboxed Windows Python the stdin handle is a pipe, so this helper is used for `os.read(0, n)` / `sys.stdin.buffer.read(n)` in the pipe fallback. It increments `consumed_lines` once per raw read regardless of how many `\n` bytes were actually consumed; small reads can satisfy `consumed_lines >= line_count` before the remaining queued REPL input runs, while large reads over multiple lines can leave the request busy. Count consumed line endings instead of treating each read call as one line. Response: Changed active stdin accounting to count consumed newline bytes and added a focused regression for partial, single-line, and multi-line reads. Finding: [P2] Normalize custom PTY CRLF before accounting — src/worker_process.rs:8030-8031 For Windows custom PTY workers, the server's active-stdin queue is built from the unnormalized payload, but this transport collapses an input `\r\n` to a single carriage return before ConPTY delivers it. Workers that normalize terminal lines back to `\n` then leave an extra `\r` in the server queue, so CRLF client input triggers a `readline_input text does not match active stdin` protocol error. Normalize the server-side payload for Windows PTY custom workers too, or preserve an accounting-equivalent sequence. Response: Added a Windows PTY protocol driver variant that normalizes input newlines for server accounting, plus unit and Zod protocol regressions for CRLF input through a custom Windows PTY worker.
Finding: [P2] Keep pipe interrupt accounting on Windows — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:211-214 On Windows this now always takes the terminal-flush path, so the old `finish_active_request_at_next_read()` cleanup no longer runs for the pipe-backed Python worker. The new launch code uses that pipe path whenever builtin Python is sandboxed, and `FlushConsoleInputBuffer` is ineffective there; if a multi-line sandboxed request times out and is interrupted before all queued lines are consumed, `discard_pending_stdin()` drains the tail but `ActiveRequest.line_count` still includes it, so the next prompt cannot satisfy the `consumed_lines >= line_count` completion check and the session can remain busy. Gate this on `windows_stdin_is_console()` or keep the finish-at-next-read path for pipe stdin. Response: Kept the terminal flush path only for Windows console stdin and restored `finish_active_request_at_next_read()` for Windows pipe stdin after drained interrupt cleanup. Added a read-only sandbox regression that times out a multi-line request, interrupts it, and verifies the follow-up request can complete without running the drained tail. Finding: [P2] Preserve raw pipe bytes on Windows — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:2032-2034 This runs every Windows raw read through the console CR/LF coalescer and returns the normalized bytes to Python. In sandboxed builtin Python the worker deliberately uses pipe stdin, not a console, so `os.read(0, n)`/`nt.read` no longer returns the bytes the client wrote, for example a raw `a\rb` payload is exposed as `a\nb`. That breaks raw pipe semantics and the previous Windows pipe path; normalize only for `windows_stdin_is_console()` and use a separate normalized copy for request accounting if needed. Response: Changed Windows raw stdin reads to apply console CR/LF coalescing only when fd 0 is a console; pipe stdin now returns the raw bytes to Python while still accounting consumed newlines. Added a read-only sandbox regression that verifies `os.read(0, ...)` preserves CRLF pipe bytes.
Finding: [P2] Drain pending direct stdin bytes on Windows interrupts — C:/Users/kalin/Documents/GitHub/mcp-repl/src/python_session.rs:547-551 On Windows PTY sessions, direct stdin reads now use `PYTHON_DIRECT_STDIN_SIDEBAND_INPUT` to hold partial UTF-8 until it can emit `readline_input`, but the Windows interrupt cleanup path never drains that buffer. If user code does something like `os.read(0, 1)` on a non-ASCII character and the request is interrupted before the remaining bytes are read, the stale byte is kept and can be prepended to the next request's accounting, producing a `readline_input text does not match active stdin` protocol error. The Windows discard path should clear and emit/discard this pending sideband buffer like the Unix path does. Response: Windows interrupt discard now drains `PYTHON_DIRECT_STDIN_SIDEBAND_INPUT` before clearing console or pipe stdin. Valid pending UTF-8 is included in the discard text; incomplete UTF-8 is cleared without emitting replacement text that cannot match the active stdin byte queue. Added focused Windows unit coverage for both invalid partial bytes and valid pending text.
Finding:
[P2] Keep request active for buffered Windows input — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:1981-1984
On Windows ConPTY `stdin_pending_byte_count()` returns `None` for console stdin, so this branch runs even when the same MCP payload already contains the answer to `input()`/`sys.stdin.readline()`. `mark_stdin_wait_prompt_completed_request()` leaves `request_completed_at_stdin_wait` set after the buffered line is read, causing plot hooks later in that same request (e.g. `x=input('p> ')
hello
plt.plot(...); plt.show()`) to treat the request as inactive and drop the image. Gate this completion on an actual unbuffered wait, or clear the completed-at-wait state when Windows reads a buffered line.
Response:
Input delivery now clears `request_completed_at_stdin_wait`, so a buffered Windows input answer reopens the request for later plot hooks. Added a state-level regression for reopening the request after stdin-wait completion and a Windows plot regression that exercises buffered input followed by `plt.show()` when plot tests are enabled.
Finding: [P2] Make Windows Python Ctrl-Break best-effort — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:6561-6561 On Windows Python backends this now turns a failed `GenerateConsoleCtrlEvent` into a hard interrupt error. In stdio-hosted MCP launches without an attached console, that API fails even though the Python interrupt is also sent through the sideband, so a user Ctrl-C can report a worker IO error and reset instead of returning to the prompt. Keep Ctrl-Break delivery best-effort for Python or gate it to cases where a console event can actually be sent. Response: Python protocol interrupts now treat the Windows Ctrl-Break delivery as best-effort after the sideband interrupt cleanup succeeds. Added a Windows regression that forces `GenerateConsoleCtrlEvent` failure and verifies the Python interrupt path still succeeds after attempting Ctrl-Break. Finding: [P2] Check PTY process liveness before exit code — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:5916-5917 Windows reserves exit code 259 as `STILL_ACTIVE`, but a process can still legally exit with that code. If a ConPTY-launched worker exits with 259, this branch treats the already-signaled process as running forever, and `wait()` can spin indefinitely because `WaitForSingleObject` keeps returning immediately. Probe the handle with a zero-timeout wait before interpreting the exit code. Response: `WindowsPtyChild::try_wait` now checks the process handle with `WaitForSingleObject(..., 0)` before treating exit code 259 as still running. Added a Windows regression that wraps a child exiting with 259 in `WindowsPtyChild` and verifies `wait()` returns that exit code.
Finding: [P2] Avoid treating every Windows pipe as stdin — C:\Users\kalin\Documents\GitHub\mcp-repl\python\embedded.py:772-772 On Windows this enables the raw-stdin bridge, but `_mcp_repl_is_raw_stdin_fd()` identifies stdin only by `st_dev`/`st_ino`; anonymous pipes on Windows commonly report both as `0`, so unrelated pipe fds (for example `subprocess.Popen(..., stdout=PIPE).stdout` or `os.read()` on another pipe) are misclassified as fd 0 and consume MCP stdin instead of the pipe output. This breaks common subprocess/pipe reads in the Windows Python backend; the Windows path needs a stronger handle identity check or a narrower fd match. Response: Narrowed Windows raw-stdin bridging to fd 0 instead of using anonymous-pipe stat identity. POSIX keeps the original fd identity behavior for duplicated stdin. Added a Windows subprocess stdout pipe regression proving `os.read()` on an unrelated pipe reads pipe output instead of consuming MCP stdin.
Finding: The Windows PTY stdin bridge misses duplicated stdin descriptors, which can wedge the session for affected Python code. Other inspected changes and targeted tests looked consistent, but this regression should be fixed before considering the patch correct. Review comment: - [P2] Track duplicated stdin fds on Windows — C:\Users\kalin\Documents\GitHub\mcp-repl\python\embedded.py:802-803 On the Windows ConPTY path, code that duplicates stdin, such as `fd = os.dup(0); os.read(fd, ...)` or `os.fdopen(os.dup(0))`, still consumes bytes from the PTY but this predicate returns false for the duplicate fd, so the sideband stdin bridge/accounting is bypassed. The server's active stdin queue then never drains for those bytes, leaving the request busy and causing later input to be discarded until reset; please track fds derived from fd 0 or otherwise route stdin aliases through the bridge. Response: Track Windows raw stdin aliases explicitly through os.dup, os.dup2, and os.close instead of relying on anonymous pipe stat identity. Added a Windows regression test that reads through os.dup(0) and verifies bridge accounting continues to drain request input.
Finding: The Windows PTY raw stdin path can surface a spurious EOF when it drops the LF half of a CRLF sequence, breaking raw stdin consumers and leaving input misrouted to the REPL. Review comment: - [P2] Avoid returning EOF for dropped CRLF bytes — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:2062-2064 On Windows PTY stdin, byte-sized raw reads can split the console CRLF for Enter across calls: after a `\r` is normalized to `\n`, the following `\n` is dropped here, leaving `protocol_bytes` empty and returning `b''` to Python. Callers treat that as EOF, so `os.read(0, 1)` loops can stop early and leave later input to be executed by the REPL (e.g. reading four bytes from `ab\nc` yields `b'a', b'b', b'\n', b''` and then `c` runs as code). Response: Keep reading Windows console stdin when CRLF normalization drops a byte and produces no protocol bytes, returning `b''` only for a real empty underlying read. Added a Windows regression test that reads four one-byte chunks across `ab\nc` and verifies the fourth read returns `b'c'` instead of EOF.
Review finding: [P2] Account split UTF-8 raw stdin reads on Windows — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:2245-2247 When Windows ConPTY user code reads raw stdin in small chunks, e.g. `os.read(0, 1)` with a non-ASCII character queued, this call can pass only the first UTF-8 byte to `emit_readline_input_bytes`; that helper buffers invalid UTF-8 and does not remove the byte from the server's active stdin queue. If the request then reaches a prompt or is interrupted before the rest of the character is read through the same path, completion never sees all stdin accounted or the next input/discard frame mismatches the active queue, leaving the session busy or returning a protocol error. Response: Added byte-preserving `readline_input_bytes` and `readline_discard_bytes` sideband accounting frames, with the same active-stdin prefix validation as text accounting. Python raw stdin accounting now emits byte frames for split or invalid UTF-8 fragments instead of keeping consumed bytes in a worker-local text buffer. Windows console stdin also shares a byte buffer between raw reads and readline reads so `os.read(0, 1)` cannot duplicate a split UTF-8 scalar when the REPL reaches the next prompt. Added protocol/unit coverage and a Windows PTY regression for split UTF-8 raw stdin.
Replace the transitional worker IPC surface with worker_ready, byte-level readline accounting, output_text, output_image, session_end, and interrupt only. Remove legacy request-boundary frames and readline_result handling, and update the Zod fixture/tests to validate raw wire-byte accounting before worker-side normalization. Fail fast for Windows sandboxed Python until sandboxed ConPTY launch can satisfy strict stdin accounting, and keep R CRLF normalization interpreter-local while reporting the original bytes. Update protocol docs and regression tests for the breaking contract.
Launch sandboxed Python through a wrapper-owned ConPTY instead of rejecting sandboxed Windows Python sessions. The server still uses pipe transport to the sandbox wrapper, and the wrapper creates the pseudoconsole for the restricted child process while preserving wire-byte accounting at the sideband contract boundary. Update docs and regression coverage for restored sandbox support and console-normalized stdin reads.
Finding: [P1] Continue sending the OS interrupt after sideband notice — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:392-394 On Unix protocol workers, including built-in Python, a successful sideband `Interrupt` now returns before `process.send_interrupt()`. The worker-side Python handler only updates bookkeeping on Unix (`request_platform_interrupt()` is a no-op), so long-running code such as `time.sleep()` or a tight loop never receives SIGINT and the session remains busy until the request finishes; custom protocol workers also stop receiving the OS interrupt that the protocol says the sideband does not replace. Response: Changed protocol interrupt routing so Windows keeps its sideband-only behavior after successful delivery, while non-Windows sends the sideband notification best-effort and continues through `process.send_interrupt()`. Added a Unix regression test that asserts both sideband interrupt delivery and SIGINT dispatch.
Full review comments: - [P1] Guard Python interrupt cleanup by request — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:159-160 On Unix Python, when a timed-out request is interrupted and the user supplies a Ctrl-C tail or immediate follow-up, the sideband interrupt can be processed after SIGINT has already returned a prompt and the server has started the next request. This handler now unconditionally drains/tcflushes stdin and marks an interrupt, so a late sideband message from the previous request can discard or interrupt the next request's bytes; the removed generation check/ack was what kept cleanup scoped to the timed-out request. - [P2] Snapshot background plots before reopening — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:1105-1109 When Python is blocked at an input/stdin wait and a background thread mutates a matplotlib figure before the user answers, this path reopens `request_active` without first recording inactive plot hashes. That leaves `_plot_hashes` stale, so the answer/no-op request can emit the background image even though that request did not plot; the removed request-start path explicitly snapshotted those background plots before reopening the gate. Response: Guarded Unix Python sideband interrupt cleanup with the current request gate so a late sideband message after the prompt boundary cannot drain or interrupt the next request's stdin. Restored background plot hash recording before stdin delivery reopens `request_active`, and added state regression tests for both request-boundary decisions.
Finding: Remove any mention of 'legacy' in the code (e.g., no legacy_stdio). These are internal only changes and we want a clean simple consistent design. Response: Removed the inert compatibility field from sandbox state updates and Codex metadata parsing, renamed internal test helpers and test cases, reworded user/test strings and planning text, and normalized Codex wire snapshots so nonessential sandbox metadata extras are not persisted. Verified with a repository-wide search for legacy terminology.
Full review comments: - [P1] Restore request generation for Python interrupts — C:\Users\kalin\Documents\GitHub\mcp-repl\src\python_session.rs:190-191 For built-in Unix Python, a sideband interrupt from a timed-out request can be handled after SIGINT has already returned Python to the prompt and the server has accepted a new request. This predicate only checks that some request is active, so a stale interrupt can run `discard_pending_stdin()`/`tcflush()` against bytes belonging to the new request; the removed generation check was the guard that prevented this cross-request drain. Response: Restored private Python interrupt generations. The built-in Python driver now tracks a request generation, sends it through a Python-only interrupt sideband message, and the Python session only performs Unix stdin discard/tcflush cleanup when the active request generation still matches. Added protocol and driver/session regression coverage for generated Python interrupts and stale generation rejection. - [P2] Honor PTY for sandboxed custom workers — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:5891-5891 When a Windows custom worker is configured with `stdin: "pty"` and runs under the server sandbox, this new PTY command wraps only the outer `mcp-repl --windows-sandbox` process. Unlike the built-in Python path, it never tells the wrapper to create a ConPTY for the restricted child, so the actual custom worker still receives ordinary pipe stdio and `isatty()`/readline-style behavior is lost under `read-only` or `workspace-write` sandboxing. Response: Sandboxed Windows custom workers that request PTY stdin now set `MCP_REPL_WINDOWS_SANDBOX_CONPTY=1` on both the process and PTY launch command so the sandbox wrapper creates a ConPTY for the restricted child. Added Windows unit coverage for the custom-worker PTY gating and env propagation.
Review finding: [P2] Deliver Windows OS interrupts after sideband notice — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:393-399 On Windows custom/protocol workers, a successful sideband `interrupt` now returns before `process.send_interrupt()`, so Ctrl-C only delivers the bookkeeping message and never sends the documented OS control event. Workers that are blocked in native code, do not poll sideband, or rely on the OS interrupt path will keep running instead of being interrupted. Response: Made protocol sideband interrupt notifications best-effort on all platforms, including Python request-generation interrupts, and always continue to the platform OS interrupt path afterward. Updated Windows regression coverage to assert both the sideband notification and Ctrl-Break delivery.
Review finding: [P1] Wait for Python interrupt cleanup before SIGINT — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:591-593 When interrupting built-in Python on Unix with unread stdin tail, such as a multi-line request that times out after Python has consumed only the sleeping line, this sends the cleanup sideband and immediately delivers SIGINT. The cleanup runs on the worker IPC thread; if SIGINT returns the REPL to a prompt before that thread drains fd 0, the old tail can be read/executed before it is discarded (or the later generation guard can skip cleanup), whereas the previous ack path waited until cleanup completed before signaling. Response: Restored the internal python_interrupt_ack message, emitted it after Python-side cleanup, and made the server wait for that cleanup acknowledgement before delivering the platform interrupt. Updated protocol direction coverage and the Python protocol interrupt unit tests to acknowledge before the OS interrupt assertion. Review finding: [P2] Forward Ctrl-Break to the sandbox ConPTY child — C:\Users\kalin\Documents\GitHub\mcp-repl\src\windows_sandbox.rs:2125-2125 When `MCP_REPL_WINDOWS_SANDBOX_CONPTY` is used for a sandboxed PTY worker, this launches the restricted child in a new process group, but the server still sends Ctrl-Break only to the wrapper process group and the wrapper's handler ignores it without forwarding. For custom PTY protocol workers that rely on the documented OS interrupt, Ctrl-C is delivered to the wrapper instead of the runtime, so the worker can keep running until the interrupt times out. Response: Added a wrapper Ctrl-Break forwarding target for ConPTY launches so the wrapper consumes Ctrl-Break locally and relays it to the restricted child process group. Added recorder-backed unit coverage for the wrapper handler and kept the sandboxed Python ConPTY interrupt regression passing.
Review finding: [P2] Preserve compatibility for protocol v1 input frames — C:/Users/kalin/Documents/GitHub/mcp-repl/src/ipc.rs:111-115 When a custom worker that advertises the existing `protocol.version: 1` sends the previously documented `readline_input` or `readline_discard` frames, this enum no longer deserializes them, so the server accepts the handshake and then fails with a sideband protocol error on the first input. Either bump the worker protocol version or keep the old text variants as compatibility aliases that are converted to bytes. Response: Kept protocol version 1 compatible by accepting legacy `readline_input.text` and `readline_discard.text` frames, accounting for their UTF-8 bytes through the same active-stdin queue used by the byte frames. Added deserialization and request-completion regression tests, and documented the compatibility aliases beside the byte-frame protocol.
Review finding: [P2] Scrub Windows IPC pipe names after connecting — C:\Users\kalin\Documents\GitHub\mcp-repl\src\ipc.rs:1520-1524 On the Windows path, once both named pipes are opened we return the `WorkerIpcConnection` without removing `MCP_REPL_IPC_PIPE_TO_WORKER` / `MCP_REPL_IPC_PIPE_FROM_WORKER`. Unlike the Unix branch above, those bootstrap variables remain visible to R/Python user code and any subprocesses in Windows sessions, which violates the single-owner sideband contract and leaks the IPC endpoints; remove both env vars after the handles are opened and before returning. Response: Removed both Windows IPC pipe-name bootstrap environment variables immediately after the worker opens both named pipe handles and before creating the worker IPC connection. Added a Windows IPC test that connects through the named-pipe bootstrap path and asserts both variables are scrubbed after connect.
Review finding: [P2] Use pipe transport for sandboxed PTY wrappers — C:\Users\kalin\Documents\GitHub\mcp-repl\src\worker_process.rs:5988-5993 On Windows, when a custom worker requests `stdin: "pty"` under a server sandbox, `custom_worker_wrapper_conpty` already asks the sandbox wrapper to create the ConPTY for the restricted child. This line still launches the wrapper itself with `spec.stdin.transport()`, so the wrapper gets an outer PTY as well; that outer console echoes/translates input before forwarding it to the inner ConPTY, producing duplicated command echoes and extra blank output for sandboxed custom PTY workers. Use pipe transport to the wrapper when `custom_worker_wrapper_conpty` is true. Response: Changed Windows sandboxed custom PTY launches to use pipe transport for the wrapper whenever the wrapper is responsible for creating the restricted child ConPTY. Added a unit test that preserves PTY transport for unsandboxed custom PTY workers while forcing pipe transport for the wrapper-ConPTY case.
Review finding: [P2] Filter sandbox-wrapper ConPTY output — C:\Users\kalin\Documents\GitHub\mcp-repl\src\windows_sandbox.rs:2355-2357 When `MCP_REPL_WINDOWS_SANDBOX_CONPTY` is enabled, the wrapper creates a ConPTY but forwards its output with `copy_wrapper_output` directly to stdout. Because the server uses the pipe reader for sandboxed wrapper launches, this path bypasses the `WindowsPtyOutputFilter` used for direct PTY launches, so sandboxed Windows Python/custom-PTY sessions can leak ConPTY cursor/OSC escape sequences into captured stdout. Response: Moved the Windows PTY output filter into a shared Windows module and applied it to sandbox-wrapper ConPTY stdout before forwarding. Added wrapper-output coverage for ConPTY cursor/OSC filtering while keeping the direct PTY filter tests.
Move R submitted-input visibility fully to sideband accounting by removing the worker stdout echo from the R console callback. This keeps the server blind to prompt/input text while preserving prompts, runtime output, and raw child stdout as ordinary output. Also mark Unix Python prompt waits complete at CPython readline boundaries so interrupt cleanup uses the current request generation and can discard buffered tails, and remove/update tests whose old purpose was preserving synthetic R echoes. Validation: - cargo check - cargo build - python3 tests/run_integration_tests.py --binary target/debug/mcp-repl - cargo clippy --all-targets --all-features -- -D warnings - cargo test --quiet - cargo +nightly fmt - cargo insta pending-snapshots
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
readline_discardaccounting.Verification
cargo checkcargo buildpython3 tests/run_integration_tests.py --binary target/debug/mcp-replcargo clippy --all-targets --all-features -- -D warningscargo test --quietcargo +nightly fmtcodex --sandbox danger-full-access --ask-for-approval never review --base main